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

[Heinrich Schuchardt]

On 10/15/19 9:34 AM, AKASHI Takahiro wrote:
> On Sat, Oct 12, 2019 at 09:43:33PM +0200, Heinrich Schuchardt wrote:
>> On 10/7/19 7:59 AM, AKASHI Takahiro wrote:
>>> Currently, a whole disk without any partitions is not associated
>>> with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses some
>>> 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 no partition
>>> table installed on it.
>>
>> That is what I would expect. But it is not what this patch really does.
>
> Literally, you're right, but
>
>> See below.
>>
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>> ---
>>>   lib/efi_loader/efi_disk.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>> index 861fcaf3747f..7ee4ed26a2ea 100644
>>> --- a/lib/efi_loader/efi_disk.c
>>> +++ b/lib/efi_loader/efi_disk.c
>>> @@ -337,7 +337,8 @@ static efi_status_t efi_disk_add_dev(
>>>   			       diskobj->dp);
>>>   	if (ret != EFI_SUCCESS)
>>>   		return ret;
>>> -	if (part >= 1 && efi_fs_exists(desc, part)) {
>>> +	/* partitions or whole disk without partitions */
>>> +	if (efi_fs_exists(desc, part)) {

The following should work:

         if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
             efi_fs_exists(desc, part)) {

But unfortunately it does not because if you format the image with
mkfs.vfat desc->part_type will be set to PART_TYPE_DOS.

So what is missing is a fix in disk/part_dos.c. Currently it only checks
for bytes 0x55 and 0xAA.

One might want to check additionally if there is either a disk
identifier at position 0x01B8 or one of the type bytes of the four
primary partitions is set. And if neither is set return -1 from
test_block_type().

Best regards

Heinrich

>>
>> Function efi_fs_exists() does not check if there is a partition table.
>> It only checks if there is a file system.
>>
>> For creating a test image you can use the following commands:
>>
>> cat > partioning << EOF
>> label: dos
>> label-id: 0x6fe3a999
>> device: image
>> unit: sectors
>> image1: start=    1024, size= 524288, type=0c
>> EOF
>> dd if=/dev/zero of=test.img count=1 bs=1MiB
>> /usr/sbin/sfdisk test.img < partioning
>> dd if=test.img of=partition.tbl bs=8 count=9 skip=55
>> /usr/sbin/mkfs.vfat test.img 1024
>> dd conv=fsync,notrunc if=partition.tbl of=test.img bs=8 count=9 seek=55
>> sudo losetup -o 524288 --sizelimit 524288 /dev/loop1 test.img
>> sudo mkfs.vfat /dev/loop1
>> sudo losetup -D /dev/loop1
>> sudo mount test.img /mnt
>> sudo sh -c "echo 'file system on block device' > /mnt/description.txt"
>> sudo umount /mnt
>> sudo mount test.img /mnt -o offset=524288
>> sudo sh -c "echo 'file system on partition 1' > /mnt/description.txt"
>> sudo umount /mnt
>
> Your example above is quite quirky, and totally unpractical.
> If people would create such a file system, they would expect
> that they could access a raw block device as well as a partition #1.
>
> However, I would like to drop this patch(#5) from my patch set
> because I don't find any public interface that can be used to
> determine if there exists a file system on a raw device.
>
> I initially thought of reverting my patch to v2, where part_get_info()
> was used, but it doesn't work as expected for part == 0.
> For example, GPT partition dirver, disk_efi.c, uses an internal
> function, find_valid_gpt(), but part_get_info_efi() returns an error
> for part == 0.
>
> For me, it is much easier to modify my pytest for UEFI secure boot
> than to carve out a generic interface for *all* the file systems.
>
> My patch #1 to #4 have already been reviewed by you and there are
> no outstanding issues.
>
> -Takahiro Akashi
>
>> Without your patch I get
>>
>> UEFI Interactive Shell v2.2
>> EDK II
>> UEFI v2.80 (Das U-Boot, 0x20191000)
>> Mapping table
>>        FS0: Alias(s):HD0a0b:;BLK1:
>>
>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/HD(1,MBR,0x6fe3a999,0x400,0x400)
>>       BLK0: Alias(s):
>>            /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
>> Press ESC in 4 seconds to skip startup.nsh or any other key to continue.
>> Shell> fs0:
>> FS0:\> cat description.txt
>> file system on partition 1
>>
>> With your patch:
>>
>> UEFI Interactive Shell v2.2
>> EDK II
>> UEFI v2.80 (Das U-Boot, 0x20191000)
>> Mapping table
>>        FS0: Alias(s):F0a0:;BLK0:
>>            /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
>>        FS1: Alias(s):HD0a0b:;BLK1:
>>
>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/HD(1,MBR,0x6fe3a999,0x400,0x400)
>> Press ESC in 4 seconds to skip startup.nsh or any other key to continue.
>> Shell> fs0:
>> FS0:\> cat description.txt
>> file system on block device
>>
>> FS0:\> fs1:
>> FS1:\> cat description.txt
>> file system on partition 1
>>
>> So though a partition table is discovered a file system is mounted on
>> the block device. In my special case the file system on the block device
>> really existed and was well separated from partition 1. But typically
>> expect that there is no file system on the block device if there is a
>> partition table.
>>
>> For your convenience I have uploaded the image file to
>> https://github.com/U-Boot-EFI/test_file_system
>>
>> Best regards
>>
>> Heinrich
>>
>>>   		diskobj->volume = efi_simple_file_system(desc, part,
>>>   							 diskobj->dp);
>>>   		ret = efi_add_protocol(&diskobj->header,
>>>
>