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

[Masami Hiramatsu]

Hi Heinrich,

2021年6月3日(木) 14:15 Heinrich Schuchardt <xypron.glpk at gmx.de>:
>
> Am 3. Juni 2021 06:57:16 MESZ schrieb Masami Hiramatsu <masami.hiramatsu at linaro.org>:
> >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,
>
> For running the SCT I use an image which has only a FAT partition.

That depends on the device configuration. My platform (DeveloperBox)
is something like PC, which has not only USB, but eMMC, SATA, NVMe. Of
course I can just disable CONFIG_EXT4 from U-Boot for SCT, but I don't
like erasing all the partitions on which I have installed debian...

>
> At least Debian and Ubuntu do not allow /boot to be on a FAT file system. If we want to boot Linux via the EFI stub without GRUB, we need ext4 support exposed to the EFI sub-system. See Ilias' recent contributions for the EFI_LOAD_FILE2_PROTOCOL for initrd and efidebug. This came in handy for booting via EFI on RISC-V where the initrd= command line parameter is not supported by Linux.

IMHO, such dependency is out of UEFI spec. That means Debian/Ubuntu
doesn't follow the UEFI spec. (but as far as I know, those install ESP
on the disk and install GRUB efi application for boot)
And yes, EFI_LOAD_FILE2_PROTOCOL needs to load initrd from somewhere
(I'm usually put it on the ESP). But, if the EFI_LOAD_FILE2_PROTOCOL
*requires* to access ext4 partition, I think that is not supported by
UEFI spec.

Anyway, I agree that denying access to non-FAT partitions is too
restricted. What about my other ideas? If the volume is set to
ReadOnly, that is good for both of the SCT and the
EFI_LOAD_FILE2_PROTOCOL.


Thank you,

>
> Best regards
>
> Heinrich
>
>
> >
> >
> >>
> >> 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