[U-Boot] [PATCH] efi_loader: disk: install file system protocol to a whole disk

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, Sep 11, 2019 at 07:31:31PM +0200, Heinrich Schuchardt wrote:
> On 9/11/19 8:16 AM, AKASHI Takahiro wrote:
> >Heinrich,
> >
> >On Fri, Aug 23, 2019 at 09:04:21AM +0900, AKASHI Takahiro wrote:
> >>On Thu, Aug 22, 2019 at 12:52:41PM +0200, Heinrich Schuchardt wrote:
> >>>On 8/22/19 11:11 AM, Mark Kettenis wrote:
> >>>>>From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>>Date: Thu, 22 Aug 2019 17:06:25 +0900
> >>>>>
> >>>>>Currently, a whole disk without any partitions is not associated
> >>>>>with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT
> >>>>>file system, there is a chance that we may not be able to access
> >>>>>it, particularly, when accesses are to be attempted after searching
> >>>>>that protocol against a device handle.
> >>>>>
> >>>>>With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed
> >>>>>to such a disk if part_get_info() shows there is not partition
> >>>>>table installed on it.
> >>>>
> >>>>Do other UEFI implementations support this?
> >>>
> >>>What use cases exist that come without partition table?
> >>
> >>I didn't find any *explicit* description in UEFI specification
> >>that mandates that any block device should have a partition table.
> >>It may be mandatory for boot(able) disks, but for others?
> >>
> >>>You can create an MBR with partition table that is a valid start of a
> >>>file system.
> >>
> >>Obviously we can do that, but if this is not a mandatory requirement,
> >>we'd better support no-partitioned cases.
> 
> I did not mean this as a requirement but as a source of errors.
> 
> My idea is that could have an MBR which by chance looks like the start
> of a file system. When you now expose this non-existent file system the
> UEFI client may create havoc.

See below.

> >
> >Any further comments?
> >
> >-Takahiro Akashi
> >
> >>
> >>>So you should first check if a partition table exists. Only if none
> >>>exists you can test for a possible file system.
> >>
> >>I don't get your point. Are you saying that we should support a file system
> >>for a disk only if it has a file system?
> >>This is not true even for existing partitions as FILE_SYSTEM PROTOCOL
> >>is always installed to every partition whether or not it really
> >>houses a file system under the current implementation.
> 
> That sounds like a bug. If a partition isn't formatted we would not even
> know whether to call the FAT or the EXT4 driver.

Yes, the issue does exist in the current implementation.
So a good approach is
1. Check if a partition table is installed or not
2. If yes,
   2-1 If a partition has a file system, install FILE_SYSTEM_PROTOCOL
       to *that* partition.
   2-2 not install FILE_SYSTEM_PROTOCOL to a whole disk
3. If no, and only if a file system exists, install FILE_SYSTEM_PROTOCOL
   to a whole disk

(2.1) and additional check at (3) should be added.

-Takahiro Akashi


> Regards
> 
> Heinrich
> 
> >>
> >>Thanks,
> >>-Takahiro Akashi
> >>
> >>
> >>>Best regards
> >>>
> >>>Heinrich
> >>>
> >>>>
> >>>>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>>---
> >>>>>  lib/efi_loader/efi_disk.c | 4 +++-
> >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>
> >>>>>diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >>>>>index 7a6b06821a47..548fe667e6f8 100644
> >>>>>--- a/lib/efi_loader/efi_disk.c
> >>>>>+++ b/lib/efi_loader/efi_disk.c
> >>>>>@@ -239,6 +239,7 @@ static efi_status_t efi_disk_add_dev(
> >>>>>  				struct efi_disk_obj **disk)
> >>>>>  {
> >>>>>  	struct efi_disk_obj *diskobj;
> >>>>>+	disk_partition_t info;
> >>>>>  	efi_status_t ret;
> >>>>>
> >>>>>  	/* Don't add empty devices */
> >>>>>@@ -270,7 +271,8 @@ static efi_status_t efi_disk_add_dev(
> >>>>>  			       diskobj->dp);
> >>>>>  	if (ret != EFI_SUCCESS)
> >>>>>  		return ret;
> >>>>>-	if (part >= 1) {
> >>>>>+	/* partitions or whole disk without partitions */
> >>>>>+	if (part >= 1 || part_get_info(desc, part, &info)) {
> >>>>>  		diskobj->volume = efi_simple_file_system(desc, part,
> >>>>>  							 diskobj->dp);
> >>>>>  		ret = efi_add_protocol(&diskobj->header,
> >>>>>--
> >>>>>2.21.0
> >>>>>
> >>>>>_______________________________________________
> >>>>>U-Boot mailing list
> >>>>>U-Boot at lists.denx.de
> >>>>>https://lists.denx.de/listinfo/u-boot
> >>>>
> >>>
> >
>