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

[Heinrich Schuchardt]

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.

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.

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,
>>