| Message ID | 20191007055939.17093-6-takahiro.akashi@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: disk: install FILE_SYSTEM_PROTOCOL to whole disk | expand |
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. See below. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@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)) { 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 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, >
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@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)) { > > 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, > >
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@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, >>> >
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)) { diskobj->volume = efi_simple_file_system(desc, part, diskobj->dp); ret = efi_add_protocol(&diskobj->header,
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. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_disk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)