[PATCH] efi: Restrict the simple file system protocol to support only FAT

[Masami Hiramatsu]

Hi Heinrich,

2021年6月3日(木) 13:08 Heinrich Schuchardt <xypron.glpk at gmx.de>:
>
> Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu <masami.hiramatsu at linaro.org>:
> >Because UEFI specification v2.9, 13.3 File System Format said "The
> >file system supported by the Extensible Firmware Interface is based
> >on the FAT file system.", the simple file system protocol might be
> >better to support only FAT filesystem.
> >
> >There must be no problem from UEFI application to access only FAT
> >because ESP must be formatted by FAT32 and the removable media is
> >FAT12 or FAT16, according to the UEFI spec.
> >
>
> Does the UEFI spec forbid to access to other file systems?

No, it does not forbid.

>
> Which problems were observed?

My problem was observed by UEFI SCT (
https://github.com/tianocore/edk2-test/tree/master/uefi-sct ), which
reported errors while testing some volumes formatted by Ext4.

I can fix that with dropping Ext4 support from the U-Boot too. Or
maybe fixed by enabling Ext4 write support. But I thought that is not
a fundamental solution because there are other filesystems which
support read-only (e.g. Btrfs). And UEFI
configuration(CONFIG_EFI_LOADER) doesn't depend on write support.

Of course, the test program itself should check whether the filesystem
is FAT (because UEFI spec only specifies FAT full support, other
filesystems are out-of-spec), but there is no way to determine that
the volume is formatted by FAT (at least in the simple filesystem
protocol). This is reasonable, because UEFI spec expects only FAT.

Thus, I have some ideas except for this fix.
- Check the filesystem driver and only if it supports full operations
(read/write, mkdir etc.), makes it available from UEFI simple file
system protocol (this also checks CONFIG_*_WRITE).
- Set the volume read only if the filesystem driver doesn't support
write and return correct error code. This will give a consistent
filesystem model to the application. (maybe SCT needs to check volume
ReadOnly flag before test it.)

What would you think?

Thank you,


>
> Best regards
>
> Heinrich
>
>
> >Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko at socionext.com>
> >Signed-off-by: Masami Hiramatsu <masami.hiramatsu at linaro.org>
> >---
> > lib/efi_loader/efi_disk.c |   18 ++++++++++++------
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> >diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >index 307d5d759b..f69ae6587f 100644
> >--- a/lib/efi_loader/efi_disk.c
> >+++ b/lib/efi_loader/efi_disk.c
> >@@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path
> >*full_path)
> > }
> >
> > /**
> >- * efi_fs_exists() - check if a partition bears a file system
> >+ * efi_supported_fs_exists() - check if a partition bears a supported
> >file system
> >  *
> >  * @desc:     block device descriptor
> >  * @part:     partition number
> >- * Return:    1 if a file system exists on the partition
> >+ * Return:    1 if a supported file system exists on the partition
> >  *            0 otherwise
> >  */
> >-static int efi_fs_exists(struct blk_desc *desc, int part)
> >+static int efi_supported_fs_exists(struct blk_desc *desc, int part)
> > {
> >       if (fs_set_blk_dev_with_part(desc, part))
> >               return 0;
> >
> >-      if (fs_get_type() == FS_TYPE_ANY)
> >+      /*
> >+       * Because UEFI specification v2.9, 13.3 File System Format said
> >+       * "The file system supported by the Extensible Firmware Interface
> >+       * is based on the FAT file system.", the simple file system protocol
> >+       * should support only FAT filesystem.
> >+       */
> >+      if (fs_get_type() != FS_TYPE_FAT)
> >               return 0;
> >
> >       fs_close();
> >@@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
> >
> >       /*
> >        * On partitions or whole disks without partitions install the
> >-       * simple file system protocol if a file system is available.
> >+       * simple file system protocol if a supported file system exists.
> >        */
> >       if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
> >-          efi_fs_exists(desc, part)) {
> >+          efi_supported_fs_exists(desc, part)) {
> >               diskobj->volume = efi_simple_file_system(desc, part,
> >                                                        diskobj->dp);
> >               ret = efi_add_protocol(&diskobj->header,
>


-- 
Masami Hiramatsu