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

[Heinrich Schuchardt]

On 6/3/21 7:39 AM, Masami Hiramatsu wrote:
> 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.
>

If a volume or file is read only the UEFI spec requires to report this
in EFI_FILE_PROTOCOL.GetInfo().

On partition level we have the following deficiencies in U-Boot:

* we cannot (re-)mount a partition read-only
* we cannot determine if a partition is read-only
* we can neither read nor write the volume label
* we cannot determine the free space.

The current implementation of the FAT driver does not support reading
attributes and dates. I have started looking into what is needed in the
FAT driver.
https://github.com/xypron/u-boot-patches/blob/bfe483ed97978678b124f8fe579682aab6e3e9d8/patch-efi-next.sh#L91
But it is not ready for submission.

Best regards

Heinrich

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