| Message ID | 162268667641.382839.15360535926598237804.stgit@localhost |
|---|---|
| State | New |
| Headers | show |
| Series | efi: Restrict the simple file system protocol to support only FAT | expand |
+Cc: Grant and Vincent This will fix some EBBR certification test errors if the target machine has any partition which is partially (e.g. read only) supported by U-Boot. UEFI spec doesn't require accessing such filesystems, but U-Boot provides. Of course, we can also drop filesystem configs except for FAT filesystem support for UEFI only platforms. But it doesn't help hybrid platforms. Thank you, 2021年6月3日(木) 11:17 Masami Hiramatsu <masami.hiramatsu@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. > > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@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
On Thu, Jun 03, 2021 at 11:17:56AM +0900, Masami Hiramatsu wrote: > 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. If I remember correctly, Heinrich rejected the idea a long time ago. When I posted the commit 867400677cda ("efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available"), he insisted that the UEFI specification does require FAT support but that it doesn't deny any support for other file systems. So I had to change the code, allowing FS_TYPE_ANY. -Takahiro Akashi > 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. > > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@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, >
Hi Akashi-san, 2021年6月3日(木) 11:50 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > On Thu, Jun 03, 2021 at 11:17:56AM +0900, Masami Hiramatsu wrote: > > 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. > > If I remember correctly, Heinrich rejected the idea a long time ago. > > When I posted the commit 867400677cda ("efi_loader: disk: install > FILE_SYSTEM_PROTOCOL only if available"), he insisted that the UEFI > specification does require FAT support but that it doesn't deny > any support for other file systems. > So I had to change the code, allowing FS_TYPE_ANY. Thanks for the good information. Indeed, the specification doesn't deny any other filesystems. I think the problem is that the simple file system protocol doesn't provide any filesystem format information. EFI_FILE_PROTOCOL.GetInfo() will get the EFI_FILE_SYSTEM_INFO, but it doesn't include the filesystem format information. Thus, UEFI application can not know why it can write the file on this volume but cannot on another volume. Maybe we can set the ReadOnly field of EFI_FILE_SYSTEM_INFO correctly if the filesystem doesn't support write operation, and make EFI_FILE_PROTOCOL.Write() returns EFI_WRITE_PROTECTED. In that case it may help UEFI application understands what happen. Thank you, > > -Takahiro Akashi > > > 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. > > > > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com> > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@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
Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@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? Which problems were observed? Best regards Heinrich >Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com> >Signed-off-by: Masami Hiramatsu <masami.hiramatsu@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,
Hi Heinrich, 2021年6月3日(木) 13:08 Heinrich Schuchardt <xypron.glpk@gmx.de>: > > Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@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@socionext.com> > >Signed-off-by: Masami Hiramatsu <masami.hiramatsu@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
Am 3. Juni 2021 06:57:16 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@linaro.org>: >Hi Heinrich, > >2021年6月3日(木) 13:08 Heinrich Schuchardt <xypron.glpk@gmx.de>: >> >> Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu ><masami.hiramatsu@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@socionext.com> >> >Signed-off-by: Masami Hiramatsu <masami.hiramatsu@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, >>
Hi Heinrich, 2021年6月3日(木) 14:15 Heinrich Schuchardt <xypron.glpk@gmx.de>: > > Am 3. Juni 2021 06:57:16 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@linaro.org>: > >Hi Heinrich, > > > >2021年6月3日(木) 13:08 Heinrich Schuchardt <xypron.glpk@gmx.de>: > >> > >> Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu > ><masami.hiramatsu@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@socionext.com> > >> >Signed-off-by: Masami Hiramatsu <masami.hiramatsu@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
On 6/3/21 7:39 AM, Masami Hiramatsu wrote: > Hi Heinrich, > > 2021年6月3日(木) 14:15 Heinrich Schuchardt <xypron.glpk@gmx.de>: >> >> Am 3. Juni 2021 06:57:16 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@linaro.org>: >>> Hi Heinrich, >>> >>> 2021年6月3日(木) 13:08 Heinrich Schuchardt <xypron.glpk@gmx.de>: >>>> >>>> Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu >>> <masami.hiramatsu@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@socionext.com> >>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@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, >>>> >> > >
[...] > > > > 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. One of the advantages in using EFI_LOAD_FILE2_PROTOCOL is that you can load it from *any* file system the firmware has access to. The only thing the kernel does is provide a buffer big enough to fit in the initrd. The firmware is free to locate the file and copy it in that memory however it sees fit. Cheers /Ilias > > 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 > > > >
Hi Ilias, 2021年6月3日(木) 15:25 Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > [...] > > > > > > 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. > > One of the advantages in using EFI_LOAD_FILE2_PROTOCOL is that you can load > it from *any* file system the firmware has access to. The only thing the > kernel does is provide a buffer big enough to fit in the initrd. The > firmware is free to locate the file and copy it in that memory however it > sees fit. Ah, I got it. Yes, EFI_LOAD_FILE2_PROTOCOL doesn't depend on the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. Thus it should be able to load the file from where the U-Boot can access. However, since current implementation depends on the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, my patch limits the ability... Thank you, > > Cheers > /Ilias > > > > 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 > > > > > > -- Masami Hiramatsu
On Thu, Jun 03, 2021 at 03:36:38PM +0900, Masami Hiramatsu wrote: > Hi Ilias, > > 2021年6月3日(木) 15:25 Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > > > [...] > > > > > > > > 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. > > > > One of the advantages in using EFI_LOAD_FILE2_PROTOCOL is that you can load > > it from *any* file system the firmware has access to. The only thing the > > kernel does is provide a buffer big enough to fit in the initrd. The > > firmware is free to locate the file and copy it in that memory however it > > sees fit. > > Ah, I got it. Yes, EFI_LOAD_FILE2_PROTOCOL doesn't depend on the > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. Thus it should be able to load > the file from where the U-Boot can access. However, since current implementation > depends on the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, my patch limits > the ability... > Yea that changed recently. On the first version, I was using u-boot internal functions to locate and load the file. When we decided to store a device path on Boot#### options, in order to locate the initrd, we started using the EFI APIs to load it. You can check commit 53f6a5aa8626 for more details. Cheers /Ilias > Thank you, > > > > > Cheers > > /Ilias > > > > > > 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 > > > > > > > > > > > > -- > Masami Hiramatsu
Hi Heinrich, 2021年6月3日(木) 15:14 Heinrich Schuchardt <xypron.glpk@gmx.de>: [..] > > If a volume or file is read only the UEFI spec requires to report this > in EFI_FILE_PROTOCOL.GetInfo(). Yes. > > 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 Hmm, but I think the filesystem driver itself defined it in fstype_info. Thus we can check it as below. fsinfo = fs_get_info(fs_get_type()); if (fsinfo->write == fs_write_unsupported) // then set the volume readonly BTW, it seems fsinfo->opendir and readdir are also required for EFI_FILE_PROTOCOL, unless it, UEFI application can not follow the directory tree. Hmm. > * 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. Ah, that's great! I would like to test it. Thank you, -- Masami Hiramatsu
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,
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. Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> --- lib/efi_loader/efi_disk.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)