| Message ID | 20171201151033.88189-1-agraf@suse.de |
|---|---|
| State | Accepted |
| Commit | c034b7fd419010116f2b6ac3b8e3b725f425d92c |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | [U-Boot] efi_loader: Fix partition offsets | expand |
On 12/01/2017 04:10 PM, Alexander Graf wrote: > Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried > to introduce the el torito scheme to all partition table types: Spawn > individual disk objects for each partition on a disk. > > Unfortunately, that code ended up creating partitions with offset=0 which meant > that anyone accessing these objects gets data from the raw block device instead > of the partition. > > Furthermore, all the el torito logic to spawn devices for partitions was > duplicated. So let's merge the two code paths and give partition disk objects > good offsets to work from, so that payloads can actually make use of them. > > Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) > Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > lib/efi_loader/efi_disk.c | 60 ++++++++++------------------------------------- > 1 file changed, 13 insertions(+), 47 deletions(-) > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 68ba2cf7b2..4e457a841b 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -264,21 +264,17 @@ out_of_memory: > printf("ERROR: Out of memory\n"); > } > > -static int efi_disk_create_eltorito(struct blk_desc *desc, > - const char *if_typename, > - int diskid, > - const char *pdevname) > +static int efi_disk_create_partitions(struct blk_desc *desc, > + const char *if_typename, > + int diskid, > + const char *pdevname) > { > int disks = 0; > -#if CONFIG_IS_ENABLED(ISO_PARTITION) > char devname[32] = { 0 }; /* dp->str is u16[32] long */ > disk_partition_t info; > int part; > > - if (desc->part_type != PART_TYPE_ISO) > - return 0; > - > - /* and devices for each partition: */ > + /* Add devices for each partition */ > for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > if (part_get_info(desc, part, &info)) > continue; > @@ -289,10 +285,6 @@ static int efi_disk_create_eltorito(struct blk_desc *desc, > disks++; > } > > - /* ... and add block device: */ > - efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0); > -#endif > - > return disks; > } > > @@ -318,31 +310,18 @@ int efi_disk_register(void) > uclass_next_device_check(&dev)) { > struct blk_desc *desc = dev_get_uclass_platdata(dev); > const char *if_typename = dev->driver->name; > - disk_partition_t info; > - int part; > > printf("Scanning disk %s...\n", dev->name); > > - /* add devices for each partition: */ > - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > - if (part_get_info(desc, part, &info)) > - continue; > - efi_disk_add_dev(dev->name, if_typename, desc, > - desc->devnum, 0, part); > - } > - > - /* ... and add block device: */ > + /* Add block device for the full device */ > efi_disk_add_dev(dev->name, if_typename, desc, > desc->devnum, 0, 0); > > disks++; > > - /* > - * El Torito images show up as block devices in an EFI world, > - * so let's create them here > - */ > - disks += efi_disk_create_eltorito(desc, if_typename, > - desc->devnum, dev->name); > + /* Partitions show up as block devices in EFI */ > + disks += efi_disk_create_partitions(desc, if_typename, > + desc->devnum, dev->name); > } > #else > int i, if_type; > @@ -361,8 +340,6 @@ int efi_disk_register(void) > for (i = 0; i < 4; i++) { include/configs/highbank.h allows 5 SCSI drives. include/configs/qemu-arm.h allows 6 SCSI drives. I guess you want to use drv->max_devs as upper limit. > struct blk_desc *desc; > char devname[32] = { 0 }; /* dp->str is u16[32] long */ > - disk_partition_t info; > - int part; > > desc = blk_get_devnum_by_type(if_type, i); > if (!desc) > @@ -373,24 +350,13 @@ int efi_disk_register(void) > snprintf(devname, sizeof(devname), "%s%d", > if_typename, i); > > - /* add devices for each partition: */ > - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > - if (part_get_info(desc, part, &info)) > - continue; > - efi_disk_add_dev(devname, if_typename, desc, > - i, 0, part); > - } > - > - /* ... and add block device: */ > + /* Add block device for the full device */ > efi_disk_add_dev(devname, if_typename, desc, i, 0, 0); > disks++; > > - /* > - * El Torito images show up as block devices > - * in an EFI world, so let's create them here > - */ > - disks += efi_disk_create_eltorito(desc, if_typename, > - i, devname); > + /* Partitions show up as block devices in EFI */ > + disks += efi_disk_create_partitions(desc, if_typename, > + i, devname); > } > } > #endif > I tested on an Odroid C2 with four partitions on an SD card (device mmc0). Device mmc1 is not installed but defined in the dtb. Bootefi selftest (with some debug output added) showed: => bootefi selftest lib/efi_loader/efi_disk.c (311) efi_disk_register, dev = 000000007df72600 Scanning disk mmc@72000.blk... lib/efi_loader/efi_disk.c (320) efi_disk_register, disks = 0 lib/efi_loader/efi_disk.c (323) efi_disk_register, disks = 1 lib/efi_loader/efi_disk.c (287) efi_disk_create_partitions, disks = 4 lib/efi_loader/efi_disk.c (328) efi_disk_register, disks = 5 Up to this point everything is ok. We created one device of the complete disk and four devices for the partions. Now it starts going wrong: Card did not respond to voltage select! mmc_init: -95, time 9 lib/efi_loader/efi_disk.c (311) efi_disk_register, dev = 000000007df72930 Scanning disk mmc@74000.blk... lib/efi_loader/efi_disk.c (320) efi_disk_register, disks = 5 lib/efi_loader/efi_disk.c (323) efi_disk_register, disks = 6 lib/efi_loader/efi_disk.c (287) efi_disk_create_partitions, disks = 0 lib/efi_loader/efi_disk.c (328) efi_disk_register, disks = 6 Found 6 disks So here we are creating a disk for an MMC device that does not physically exist. Can't we check the existence? I wonder why uclass_next_device_check does not skip the drive. Only the following MMC related device paths are created: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0)/HD(Part0,Sig6fe30000) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0)/HD(Part1,Sig6fe30000) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0)/HD(Part2,Sig6fe30000) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MMC(Slot0)/HD(Part3,Sig6fe30000) The device paths with partition number 0 for the disk devices are missing and the partitions incorrectly numbered 0 to 3 instead of 1 to 4. But I guess this problem is in another part of the code. Best regards Heinrich
> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried > to introduce the el torito scheme to all partition table types: Spawn > individual disk objects for each partition on a disk. > > Unfortunately, that code ended up creating partitions with offset=0 which meant > that anyone accessing these objects gets data from the raw block device instead > of the partition. > > Furthermore, all the el torito logic to spawn devices for partitions was > duplicated. So let's merge the two code paths and give partition disk objects > good offsets to work from, so that payloads can actually make use of them. > > Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) > Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> > Signed-off-by: Alexander Graf <agraf@suse.de> Thanks, applied to efi-next Alex
On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: > Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried > to introduce the el torito scheme to all partition table types: Spawn > individual disk objects for each partition on a disk. > > Unfortunately, that code ended up creating partitions with offset=0 which meant > that anyone accessing these objects gets data from the raw block device instead > of the partition. > > Furthermore, all the el torito logic to spawn devices for partitions was > duplicated. So let's merge the two code paths and give partition disk objects > good offsets to work from, so that payloads can actually make use of them. > > Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) > Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> > Signed-off-by: Alexander Graf <agraf@suse.de> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE node with the loaded image protocol on rpi_3 with mmc/usb. broken ## Starting EFI application at 01000000 ... Scanning disk sdhci@7e300000.blk... efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0 efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:1, if_typename=mmc_blk, dev_index=0 offset=8192, part=1 efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:4, if_typename=mmc_blk, dev_index=0 offset=16384, part=4 Scanning disk usb_mass_storage.lun0... efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0 efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:1, if_typename=usb_storage_blk, dev_index=0 offset=8192, part=1 efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:4, if_typename=usb_storage_blk, dev_index=0 offset=40960, part=4 Found 6 disks efi_dp_match a: len 20 type 1 b: len 20 type: 1 efi_dp_match a: len 20 type 1 b: len 20 type: 1 efi_dp_match a: len 20 type 1 b: len 20 type: 1 efi_dp_match a: len 20 type 1 b: len 20 type: 1 efi_dp_match a: len 11 type 3 b: len 11 type: 3 efi_dp_match a: len 11 type 3 b: len 11 type: 3 efi_dp_match a: len 11 type 3 b: len 11 type: 3 efi_diskprobe efi_device_path_depth looking for type 4 i=0 type 1 efi_device_path_depth looking for type 4 i=1 type 3 efi_device_path_depth looking for type 4 i=2 type 3 efi_device_path_depth looking for type 4 i=3 type 3 efi_device_path_depth no match for type 4 depth=-1 working (this commit reverted) ## Starting EFI application at 01000000 ... Scanning disk sdhci@7e300000.blk... efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=1 efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=4 efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0 Scanning disk usb_mass_storage.lun0... efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=1 efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=4 efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0 Found 2 disks efi_dp_match a: len 20 type 1 b: len 20 type: 1 efi_dp_match a: len 20 type 1 b: len 20 type: 1 efi_dp_match a: len 20 type 1 b: len 20 type: 1 efi_dp_match a: len 20 type 1 b: len 20 type: 1 efi_dp_match a: len 11 type 3 b: len 11 type: 3 efi_dp_match a: len 11 type 3 b: len 11 type: 3 efi_dp_match a: len 11 type 3 b: len 11 type: 3 efi_dp_match a: len 42 type 4 b: len 42 type: 4 efi_diskprobe efi_device_path_depth looking for type 4 i=0 type 1 efi_device_path_depth looking for type 4 i=1 type 3 efi_device_path_depth looking for type 4 i=2 type 3 efi_device_path_depth looking for type 4 i=3 type 3 efi_device_path_depth looking for type 4 i=4 type 4 depth=4 > --- > lib/efi_loader/efi_disk.c | 60 ++++++++++------------------------------------- > 1 file changed, 13 insertions(+), 47 deletions(-) > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 68ba2cf7b2..4e457a841b 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -264,21 +264,17 @@ out_of_memory: > printf("ERROR: Out of memory\n"); > } > > -static int efi_disk_create_eltorito(struct blk_desc *desc, > - const char *if_typename, > - int diskid, > - const char *pdevname) > +static int efi_disk_create_partitions(struct blk_desc *desc, > + const char *if_typename, > + int diskid, > + const char *pdevname) > { > int disks = 0; > -#if CONFIG_IS_ENABLED(ISO_PARTITION) > char devname[32] = { 0 }; /* dp->str is u16[32] long */ > disk_partition_t info; > int part; > > - if (desc->part_type != PART_TYPE_ISO) > - return 0; > - > - /* and devices for each partition: */ > + /* Add devices for each partition */ > for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > if (part_get_info(desc, part, &info)) > continue; > @@ -289,10 +285,6 @@ static int efi_disk_create_eltorito(struct blk_desc *desc, > disks++; > } > > - /* ... and add block device: */ > - efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0); > -#endif > - > return disks; > } > > @@ -318,31 +310,18 @@ int efi_disk_register(void) > uclass_next_device_check(&dev)) { > struct blk_desc *desc = dev_get_uclass_platdata(dev); > const char *if_typename = dev->driver->name; > - disk_partition_t info; > - int part; > > printf("Scanning disk %s...\n", dev->name); > > - /* add devices for each partition: */ > - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > - if (part_get_info(desc, part, &info)) > - continue; > - efi_disk_add_dev(dev->name, if_typename, desc, > - desc->devnum, 0, part); > - } > - > - /* ... and add block device: */ > + /* Add block device for the full device */ > efi_disk_add_dev(dev->name, if_typename, desc, > desc->devnum, 0, 0); > > disks++; > > - /* > - * El Torito images show up as block devices in an EFI world, > - * so let's create them here > - */ > - disks += efi_disk_create_eltorito(desc, if_typename, > - desc->devnum, dev->name); > + /* Partitions show up as block devices in EFI */ > + disks += efi_disk_create_partitions(desc, if_typename, > + desc->devnum, dev->name); > } > #else > int i, if_type; > @@ -361,8 +340,6 @@ int efi_disk_register(void) > for (i = 0; i < 4; i++) { > struct blk_desc *desc; > char devname[32] = { 0 }; /* dp->str is u16[32] long */ > - disk_partition_t info; > - int part; > > desc = blk_get_devnum_by_type(if_type, i); > if (!desc) > @@ -373,24 +350,13 @@ int efi_disk_register(void) > snprintf(devname, sizeof(devname), "%s%d", > if_typename, i); > > - /* add devices for each partition: */ > - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > - if (part_get_info(desc, part, &info)) > - continue; > - efi_disk_add_dev(devname, if_typename, desc, > - i, 0, part); > - } > - > - /* ... and add block device: */ > + /* Add block device for the full device */ > efi_disk_add_dev(devname, if_typename, desc, i, 0, 0); > disks++; > > - /* > - * El Torito images show up as block devices > - * in an EFI world, so let's create them here > - */ > - disks += efi_disk_create_eltorito(desc, if_typename, > - i, devname); > + /* Partitions show up as block devices in EFI */ > + disks += efi_disk_create_partitions(desc, if_typename, > + i, devname); > } > } > #endif > -- > 2.12.3 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On 07.12.17 08:00, Jonathan Gray wrote: > On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: >> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried >> to introduce the el torito scheme to all partition table types: Spawn >> individual disk objects for each partition on a disk. >> >> Unfortunately, that code ended up creating partitions with offset=0 which meant >> that anyone accessing these objects gets data from the raw block device instead >> of the partition. >> >> Furthermore, all the el torito logic to spawn devices for partitions was >> duplicated. So let's merge the two code paths and give partition disk objects >> good offsets to work from, so that payloads can actually make use of them. >> >> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) >> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> >> Signed-off-by: Alexander Graf <agraf@suse.de> > > This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE > node with the loaded image protocol on rpi_3 with mmc/usb. Hooray :). Do you think you could somehow assemble a test that runs inside Travis-CI to make sure we catch your loader? I still need to do a better one for grub to ensure that that one's happy too. > > broken > ## Starting EFI application at 01000000 ... > Scanning disk sdhci@7e300000.blk... > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0 > efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:1, if_typename=mmc_blk, dev_index=0 offset=8192, part=1 > efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:4, if_typename=mmc_blk, dev_index=0 offset=16384, part=4 > Scanning disk usb_mass_storage.lun0... > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0 > efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:1, if_typename=usb_storage_blk, dev_index=0 offset=8192, part=1 > efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:4, if_typename=usb_storage_blk, dev_index=0 offset=40960, part=4 > Found 6 disks > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > efi_diskprobe > efi_device_path_depth looking for type 4 i=0 type 1 > efi_device_path_depth looking for type 4 i=1 type 3 > efi_device_path_depth looking for type 4 i=2 type 3 > efi_device_path_depth looking for type 4 i=3 type 3 > efi_device_path_depth no match for type 4 > depth=-1 I can reproduce this with grub here as well - the device path passed in via loaded image seems to have something to it that's wrong. I can't quite figure out what - printing it with our own helpers seems to show a correct path: do_bootefi_exec: dp=/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Usb(0x6,0x0)/HD(Part1,Sigcb08e82d) I'll need to dig a bit further into this to understand where things are going wrong... Alex
On 12/07/2017 08:00 AM, Jonathan Gray wrote: > On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: >> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried >> to introduce the el torito scheme to all partition table types: Spawn >> individual disk objects for each partition on a disk. >> >> Unfortunately, that code ended up creating partitions with offset=0 which meant >> that anyone accessing these objects gets data from the raw block device instead >> of the partition. >> >> Furthermore, all the el torito logic to spawn devices for partitions was >> duplicated. So let's merge the two code paths and give partition disk objects >> good offsets to work from, so that payloads can actually make use of them. >> >> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) >> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> >> Signed-off-by: Alexander Graf <agraf@suse.de> > > This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE > node with the loaded image protocol on rpi_3 with mmc/usb. Could you, please, specify which software you are trying to call: Linux EFI stub, Grub, or anything else? Which patches did you consider? Did you apply these patch series that are not yet in efi-next? efi_loader: correct media device paths efi_loader: avoid use after free Regards Heinrich
On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote: > On 12/07/2017 08:00 AM, Jonathan Gray wrote: > > On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: > >> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried > >> to introduce the el torito scheme to all partition table types: Spawn > >> individual disk objects for each partition on a disk. > >> > >> Unfortunately, that code ended up creating partitions with offset=0 which meant > >> that anyone accessing these objects gets data from the raw block device instead > >> of the partition. > >> > >> Furthermore, all the el torito logic to spawn devices for partitions was > >> duplicated. So let's merge the two code paths and give partition disk objects > >> good offsets to work from, so that payloads can actually make use of them. > >> > >> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) > >> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> > >> Signed-off-by: Alexander Graf <agraf@suse.de> > > > > This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE > > node with the loaded image protocol on rpi_3 with mmc/usb. > > Could you, please, specify which software you are trying to call: > Linux EFI stub, Grub, or anything else? https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/ Disk image with fat/ffs filesystems https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs though it would likely show up on other archs as well armv7 equivalents of the above https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/ https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs > > Which patches did you consider? > Did you apply these patch series that are not yet in efi-next? > efi_loader: correct media device paths > efi_loader: avoid use after free just master c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path with a small patch to force calling gnu sed for some non-portable sed use in check-config.sh 'efi_loader: avoid use after free' doesn't help 'efi_loader: correct media device paths' doesn't either > > Regards > > Heinrich
On 07.12.17 12:45, Jonathan Gray wrote: > On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote: >> On 12/07/2017 08:00 AM, Jonathan Gray wrote: >>> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: >>>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried >>>> to introduce the el torito scheme to all partition table types: Spawn >>>> individual disk objects for each partition on a disk. >>>> >>>> Unfortunately, that code ended up creating partitions with offset=0 which meant >>>> that anyone accessing these objects gets data from the raw block device instead >>>> of the partition. >>>> >>>> Furthermore, all the el torito logic to spawn devices for partitions was >>>> duplicated. So let's merge the two code paths and give partition disk objects >>>> good offsets to work from, so that payloads can actually make use of them. >>>> >>>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) >>>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> >>>> Signed-off-by: Alexander Graf <agraf@suse.de> >>> >>> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE >>> node with the loaded image protocol on rpi_3 with mmc/usb. >> >> Could you, please, specify which software you are trying to call: >> Linux EFI stub, Grub, or anything else? > > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/ > Disk image with fat/ffs filesystems > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs > > though it would likely show up on other archs as well > > armv7 equivalents of the above > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/ > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs > >> >> Which patches did you consider? >> Did you apply these patch series that are not yet in efi-next? >> efi_loader: correct media device paths >> efi_loader: avoid use after free > > just master > c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path > > with a small patch to force calling gnu sed for some non-portable > sed use in check-config.sh > > 'efi_loader: avoid use after free' doesn't help > 'efi_loader: correct media device paths' doesn't either As a quick heads-up: The device path matching is broken. The patch below should fix it, but I want to create a travis-ci case around that first and also wrap it up more nicely. Alex diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index b4e2f933cb..d12a38c450 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -126,6 +126,7 @@ static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path, struct efi_device_path **rem) { struct efi_object *efiobj; + unsigned int dp_size = efi_dp_size(dp); list_for_each_entry(efiobj, &efi_obj_list, link) { struct efi_handler *handler; @@ -141,10 +142,18 @@ static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path, do { if (efi_dp_match(dp, obj_dp) == 0) { if (rem) { + /* + * Allow partial matches, but inform + * the caller. + */ *rem = ((void *)dp) + efi_dp_size(obj_dp); + return efiobj; + } else { + /* Only return on exact matches */ + if (efi_dp_size(obj_dp) == dp_size) + return efiobj; } - return efiobj; } obj_dp = shorten_path(efi_dp_next(obj_dp)); @@ -164,8 +173,14 @@ struct efi_object *efi_dp_find_obj(struct efi_device_path *dp, { struct efi_object *efiobj; - efiobj = find_obj(dp, false, rem); + /* Search for an exact match first */ + efiobj = find_obj(dp, false, NULL); + /* Then for a fuzzy match */ + if (!efiobj) + efiobj = find_obj(dp, false, rem); + + /* And now for a fuzzy short match */ if (!efiobj) efiobj = find_obj(dp, true, rem);
On Fri, Dec 08, 2017 at 06:55:02AM +0100, Alexander Graf wrote: > > > On 07.12.17 12:45, Jonathan Gray wrote: > > On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote: > >> On 12/07/2017 08:00 AM, Jonathan Gray wrote: > >>> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: > >>>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried > >>>> to introduce the el torito scheme to all partition table types: Spawn > >>>> individual disk objects for each partition on a disk. > >>>> > >>>> Unfortunately, that code ended up creating partitions with offset=0 which meant > >>>> that anyone accessing these objects gets data from the raw block device instead > >>>> of the partition. > >>>> > >>>> Furthermore, all the el torito logic to spawn devices for partitions was > >>>> duplicated. So let's merge the two code paths and give partition disk objects > >>>> good offsets to work from, so that payloads can actually make use of them. > >>>> > >>>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) > >>>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> > >>>> Signed-off-by: Alexander Graf <agraf@suse.de> > >>> > >>> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE > >>> node with the loaded image protocol on rpi_3 with mmc/usb. > >> > >> Could you, please, specify which software you are trying to call: > >> Linux EFI stub, Grub, or anything else? > > > > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI > > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/ > > Disk image with fat/ffs filesystems > > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs > > > > though it would likely show up on other archs as well > > > > armv7 equivalents of the above > > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI > > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/ > > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs > > > >> > >> Which patches did you consider? > >> Did you apply these patch series that are not yet in efi-next? > >> efi_loader: correct media device paths > >> efi_loader: avoid use after free > > > > just master > > c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path > > > > with a small patch to force calling gnu sed for some non-portable > > sed use in check-config.sh > > > > 'efi_loader: avoid use after free' doesn't help > > 'efi_loader: correct media device paths' doesn't either > > As a quick heads-up: The device path matching is broken. The patch below > should fix it, but I want to create a travis-ci case around that first > and also wrap it up more nicely. > > Alex Yes, this does fix it. Thanks. Your mail client wrapped lines so I had to adjust it to apply. Including it again with that fixed: diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index b4e2f933cb..24a4f40c00 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -126,6 +126,7 @@ static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path, struct efi_device_path **rem) { struct efi_object *efiobj; + unsigned int dp_size = efi_dp_size(dp); list_for_each_entry(efiobj, &efi_obj_list, link) { struct efi_handler *handler; @@ -141,10 +142,18 @@ static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path, do { if (efi_dp_match(dp, obj_dp) == 0) { if (rem) { + /* + * Allow partial matches, but inform + * the caller. + */ *rem = ((void *)dp) + efi_dp_size(obj_dp); + return efiobj; + } else { + /* Only return on exact matches */ + if (efi_dp_size(obj_dp) == dp_size) + return efiobj; } - return efiobj; } obj_dp = shorten_path(efi_dp_next(obj_dp)); @@ -164,8 +173,14 @@ struct efi_object *efi_dp_find_obj(struct efi_device_path *dp, { struct efi_object *efiobj; - efiobj = find_obj(dp, false, rem); + /* Search for an exact match first */ + efiobj = find_obj(dp, false, NULL); + /* Then for a fuzzy match */ + if (!efiobj) + efiobj = find_obj(dp, false, rem); + + /* And now for a fuzzy short match */ if (!efiobj) efiobj = find_obj(dp, true, rem);
On 12/08/2017 06:55 AM, Alexander Graf wrote: > > > On 07.12.17 12:45, Jonathan Gray wrote: >> On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote: >>> On 12/07/2017 08:00 AM, Jonathan Gray wrote: >>>> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: >>>>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried >>>>> to introduce the el torito scheme to all partition table types: Spawn >>>>> individual disk objects for each partition on a disk. >>>>> >>>>> Unfortunately, that code ended up creating partitions with offset=0 which meant >>>>> that anyone accessing these objects gets data from the raw block device instead >>>>> of the partition. >>>>> >>>>> Furthermore, all the el torito logic to spawn devices for partitions was >>>>> duplicated. So let's merge the two code paths and give partition disk objects >>>>> good offsets to work from, so that payloads can actually make use of them. >>>>> >>>>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) >>>>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> >>>>> Signed-off-by: Alexander Graf <agraf@suse.de> >>>> >>>> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE >>>> node with the loaded image protocol on rpi_3 with mmc/usb. >>> >>> Could you, please, specify which software you are trying to call: >>> Linux EFI stub, Grub, or anything else? >> >> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI >> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/ >> Disk image with fat/ffs filesystems >> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs >> >> though it would likely show up on other archs as well >> >> armv7 equivalents of the above >> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI >> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/ >> https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs >> >>> >>> Which patches did you consider? >>> Did you apply these patch series that are not yet in efi-next? >>> efi_loader: correct media device paths >>> efi_loader: avoid use after free >> >> just master >> c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path >> >> with a small patch to force calling gnu sed for some non-portable >> sed use in check-config.sh >> >> 'efi_loader: avoid use after free' doesn't help >> 'efi_loader: correct media device paths' doesn't either > > As a quick heads-up: The device path matching is broken. The patch below > should fix it, but I want to create a travis-ci case around that first > and also wrap it up more nicely. > > Alex > > > diff --git a/lib/efi_loader/efi_device_path.c > b/lib/efi_loader/efi_device_path.c > index b4e2f933cb..d12a38c450 100644 > --- a/lib/efi_loader/efi_device_path.c > +++ b/lib/efi_loader/efi_device_path.c Hello Alex, thank you for your further investigations We really need comments above the functions stating what they are expected to do. > @@ -126,6 +126,7 @@ static struct efi_object *find_obj(struct > efi_device_path *dp, bool short_path, > struct efi_device_path **rem) > { > struct efi_object *efiobj; > + unsigned int dp_size = efi_dp_size(dp); > > list_for_each_entry(efiobj, &efi_obj_list, link) { > struct efi_handler *handler; > @@ -141,10 +142,18 @@ static struct efi_object *find_obj(struct > efi_device_path *dp, bool short_path, > do { > if (efi_dp_match(dp, obj_dp) == 0) { > if (rem) { > + /* > + * Allow partial matches, but inform > + * the caller. > + */ > *rem = ((void *)dp) + > efi_dp_size(obj_dp); > + return efiobj; > + } else { > + /* Only return on exact matches */ > + if (efi_dp_size(obj_dp) == dp_size) > + return efiobj; > } > - return efiobj; > } > > obj_dp = shorten_path(efi_dp_next(obj_dp)); > @@ -164,8 +173,14 @@ struct efi_object *efi_dp_find_obj(struct > efi_device_path *dp, > { > struct efi_object *efiobj; > > - efiobj = find_obj(dp, false, rem); > + /* Search for an exact match first */ > + efiobj = find_obj(dp, false, NULL); > > + /* Then for a fuzzy match */ > + if (!efiobj) > + efiobj = find_obj(dp, false, rem); > + > + /* And now for a fuzzy short match */ > if (!efiobj) > efiobj = find_obj(dp, true, rem); > Unfortunately after your patch some problems with device paths still remain: I applied your patch after my patch series. I ran qemu-x86_defconfig with an IDE disk. To display the device paths I used 'bootefi selftest'. With and without your patch I saw a device path /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/HD(1,MBR,0x986b6db1,0x800,0x3f800) Between VenHW and HD the device path node for the IDE disk is missing. We should have something like /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Ata(0,Master,0)/HD(1,MBR,0x986b6db1,0x800,0x3f800) iPXE reports an error because the parent of the partition is not a block device. We should add to efi_selftest a test requiring: The parent of a partition HD(* and the partition itself must expose the EFI_BLOCK_IO_PROTOCOL. Cf. UEFI Spec 10.4.5 Media Device Path Rules The root node should not expose the EFI_BLOCK_IO_PROTOCOL. We should further add a test requiring: A partition HD(* with non-zero partition number must have a positive offset and size. Best regards Heinrich
On 12/07/2017 08:00 AM, Jonathan Gray wrote: > On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: >> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried >> to introduce the el torito scheme to all partition table types: Spawn >> individual disk objects for each partition on a disk. >> >> Unfortunately, that code ended up creating partitions with offset=0 which meant >> that anyone accessing these objects gets data from the raw block device instead >> of the partition. Hello Jonathan, according to the UEFI standard the whole disk may be represented by as partition number 0 (with offset 0). UEFI spec 2.7: 10.3.6.1 Hard Drive A partition number of zero can be used to represent the raw hard drive or a raw extended partition. Do you have access to the source of the BSD loader that you are calling? Please, check that it complies to this aspect of the UEFI API. Best regards Heinrich >> >> Furthermore, all the el torito logic to spawn devices for partitions was >> duplicated. So let's merge the two code paths and give partition disk objects >> good offsets to work from, so that payloads can actually make use of them. >> >> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) >> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> >> Signed-off-by: Alexander Graf <agraf@suse.de> > > This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE > node with the loaded image protocol on rpi_3 with mmc/usb. > > broken > ## Starting EFI application at 01000000 ... > Scanning disk sdhci@7e300000.blk... > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0 > efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:1, if_typename=mmc_blk, dev_index=0 offset=8192, part=1 > efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:4, if_typename=mmc_blk, dev_index=0 offset=16384, part=4 > Scanning disk usb_mass_storage.lun0... > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0 > efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:1, if_typename=usb_storage_blk, dev_index=0 offset=8192, part=1 > efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:4, if_typename=usb_storage_blk, dev_index=0 offset=40960, part=4 > Found 6 disks > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > efi_diskprobe > efi_device_path_depth looking for type 4 i=0 type 1 > efi_device_path_depth looking for type 4 i=1 type 3 > efi_device_path_depth looking for type 4 i=2 type 3 > efi_device_path_depth looking for type 4 i=3 type 3 > efi_device_path_depth no match for type 4 > depth=-1 > > working (this commit reverted) > ## Starting EFI application at 01000000 ... > Scanning disk sdhci@7e300000.blk... > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=1 > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=4 > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0 > Scanning disk usb_mass_storage.lun0... > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=1 > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=4 > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0 > Found 2 disks > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > efi_dp_match a: len 42 type 4 b: len 42 type: 4 > efi_diskprobe > efi_device_path_depth looking for type 4 i=0 type 1 > efi_device_path_depth looking for type 4 i=1 type 3 > efi_device_path_depth looking for type 4 i=2 type 3 > efi_device_path_depth looking for type 4 i=3 type 3 > efi_device_path_depth looking for type 4 i=4 type 4 > depth=4 > >> --- >> lib/efi_loader/efi_disk.c | 60 ++++++++++------------------------------------- >> 1 file changed, 13 insertions(+), 47 deletions(-) >> >> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c >> index 68ba2cf7b2..4e457a841b 100644 >> --- a/lib/efi_loader/efi_disk.c >> +++ b/lib/efi_loader/efi_disk.c >> @@ -264,21 +264,17 @@ out_of_memory: >> printf("ERROR: Out of memory\n"); >> } >> >> -static int efi_disk_create_eltorito(struct blk_desc *desc, >> - const char *if_typename, >> - int diskid, >> - const char *pdevname) >> +static int efi_disk_create_partitions(struct blk_desc *desc, >> + const char *if_typename, >> + int diskid, >> + const char *pdevname) >> { >> int disks = 0; >> -#if CONFIG_IS_ENABLED(ISO_PARTITION) >> char devname[32] = { 0 }; /* dp->str is u16[32] long */ >> disk_partition_t info; >> int part; >> >> - if (desc->part_type != PART_TYPE_ISO) >> - return 0; >> - >> - /* and devices for each partition: */ >> + /* Add devices for each partition */ >> for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { >> if (part_get_info(desc, part, &info)) >> continue; >> @@ -289,10 +285,6 @@ static int efi_disk_create_eltorito(struct blk_desc *desc, >> disks++; >> } >> >> - /* ... and add block device: */ >> - efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0); >> -#endif >> - >> return disks; >> } >> >> @@ -318,31 +310,18 @@ int efi_disk_register(void) >> uclass_next_device_check(&dev)) { >> struct blk_desc *desc = dev_get_uclass_platdata(dev); >> const char *if_typename = dev->driver->name; >> - disk_partition_t info; >> - int part; >> >> printf("Scanning disk %s...\n", dev->name); >> >> - /* add devices for each partition: */ >> - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { >> - if (part_get_info(desc, part, &info)) >> - continue; >> - efi_disk_add_dev(dev->name, if_typename, desc, >> - desc->devnum, 0, part); >> - } >> - >> - /* ... and add block device: */ >> + /* Add block device for the full device */ >> efi_disk_add_dev(dev->name, if_typename, desc, >> desc->devnum, 0, 0); >> >> disks++; >> >> - /* >> - * El Torito images show up as block devices in an EFI world, >> - * so let's create them here >> - */ >> - disks += efi_disk_create_eltorito(desc, if_typename, >> - desc->devnum, dev->name); >> + /* Partitions show up as block devices in EFI */ >> + disks += efi_disk_create_partitions(desc, if_typename, >> + desc->devnum, dev->name); >> } >> #else >> int i, if_type; >> @@ -361,8 +340,6 @@ int efi_disk_register(void) >> for (i = 0; i < 4; i++) { >> struct blk_desc *desc; >> char devname[32] = { 0 }; /* dp->str is u16[32] long */ >> - disk_partition_t info; >> - int part; >> >> desc = blk_get_devnum_by_type(if_type, i); >> if (!desc) >> @@ -373,24 +350,13 @@ int efi_disk_register(void) >> snprintf(devname, sizeof(devname), "%s%d", >> if_typename, i); >> >> - /* add devices for each partition: */ >> - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { >> - if (part_get_info(desc, part, &info)) >> - continue; >> - efi_disk_add_dev(devname, if_typename, desc, >> - i, 0, part); >> - } >> - >> - /* ... and add block device: */ >> + /* Add block device for the full device */ >> efi_disk_add_dev(devname, if_typename, desc, i, 0, 0); >> disks++; >> >> - /* >> - * El Torito images show up as block devices >> - * in an EFI world, so let's create them here >> - */ >> - disks += efi_disk_create_eltorito(desc, if_typename, >> - i, devname); >> + /* Partitions show up as block devices in EFI */ >> + disks += efi_disk_create_partitions(desc, if_typename, >> + i, devname); >> } >> } >> #endif >> -- >> 2.12.3 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> https://lists.denx.de/listinfo/u-boot >
On Fri, Dec 08, 2017 at 08:09:46AM +0100, Heinrich Schuchardt wrote: > On 12/07/2017 08:00 AM, Jonathan Gray wrote: > > On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: > > > Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried > > > to introduce the el torito scheme to all partition table types: Spawn > > > individual disk objects for each partition on a disk. > > > > > > Unfortunately, that code ended up creating partitions with offset=0 which meant > > > that anyone accessing these objects gets data from the raw block device instead > > > of the partition. > > Hello Jonathan, > > according to the UEFI standard the whole disk may be represented by as > partition number 0 (with offset 0). You are quoting Alexander there not me. > > UEFI spec 2.7: > 10.3.6.1 Hard Drive > A partition number of zero can be used to represent the raw hard drive or a > raw extended partition. > > Do you have access to the source of the BSD loader that you are calling? > Please, check that it complies to this aspect of the UEFI API. Already mentioned in this thread, but again: https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/ Which works fine with the EDK2 based firmware on the overdrive 1000. And has found various regressions in changes made to the efi loader in U-Boot over the last few months. > > Best regards > > Heinrich > > > > > > > > Furthermore, all the el torito logic to spawn devices for partitions was > > > duplicated. So let's merge the two code paths and give partition disk objects > > > good offsets to work from, so that payloads can actually make use of them. > > > > > > Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) > > > Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> > > > Signed-off-by: Alexander Graf <agraf@suse.de> > > > > This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE > > node with the loaded image protocol on rpi_3 with mmc/usb. > > > > broken > > ## Starting EFI application at 01000000 ... > > Scanning disk sdhci@7e300000.blk... > > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0 > > efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:1, if_typename=mmc_blk, dev_index=0 offset=8192, part=1 > > efi_disk_create_partitions efi_disk_add_dev name=sdhci@7e300000.blk:4, if_typename=mmc_blk, dev_index=0 offset=16384, part=4 > > Scanning disk usb_mass_storage.lun0... > > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0 > > efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:1, if_typename=usb_storage_blk, dev_index=0 offset=8192, part=1 > > efi_disk_create_partitions efi_disk_add_dev name=usb_mass_storage.lun0:4, if_typename=usb_storage_blk, dev_index=0 offset=40960, part=4 > > Found 6 disks > > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > > efi_diskprobe > > efi_device_path_depth looking for type 4 i=0 type 1 > > efi_device_path_depth looking for type 4 i=1 type 3 > > efi_device_path_depth looking for type 4 i=2 type 3 > > efi_device_path_depth looking for type 4 i=3 type 3 > > efi_device_path_depth no match for type 4 > > depth=-1 > > > > working (this commit reverted) > > ## Starting EFI application at 01000000 ... > > Scanning disk sdhci@7e300000.blk... > > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=1 > > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=4 > > efi_disk_register BLK efi_disk_add_dev name=sdhci@7e300000.blk, if_typename=mmc_blk, dev_index=0 offset=0, part=0 > > Scanning disk usb_mass_storage.lun0... > > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=1 > > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=4 > > efi_disk_register BLK efi_disk_add_dev name=usb_mass_storage.lun0, if_typename=usb_storage_blk, dev_index=0 offset=0, part=0 > > Found 2 disks > > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > > efi_dp_match a: len 20 type 1 b: len 20 type: 1 > > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > > efi_dp_match a: len 11 type 3 b: len 11 type: 3 > > efi_dp_match a: len 42 type 4 b: len 42 type: 4 > > efi_diskprobe > > efi_device_path_depth looking for type 4 i=0 type 1 > > efi_device_path_depth looking for type 4 i=1 type 3 > > efi_device_path_depth looking for type 4 i=2 type 3 > > efi_device_path_depth looking for type 4 i=3 type 3 > > efi_device_path_depth looking for type 4 i=4 type 4 > > depth=4 > > > > > --- > > > lib/efi_loader/efi_disk.c | 60 ++++++++++------------------------------------- > > > 1 file changed, 13 insertions(+), 47 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > > index 68ba2cf7b2..4e457a841b 100644 > > > --- a/lib/efi_loader/efi_disk.c > > > +++ b/lib/efi_loader/efi_disk.c > > > @@ -264,21 +264,17 @@ out_of_memory: > > > printf("ERROR: Out of memory\n"); > > > } > > > -static int efi_disk_create_eltorito(struct blk_desc *desc, > > > - const char *if_typename, > > > - int diskid, > > > - const char *pdevname) > > > +static int efi_disk_create_partitions(struct blk_desc *desc, > > > + const char *if_typename, > > > + int diskid, > > > + const char *pdevname) > > > { > > > int disks = 0; > > > -#if CONFIG_IS_ENABLED(ISO_PARTITION) > > > char devname[32] = { 0 }; /* dp->str is u16[32] long */ > > > disk_partition_t info; > > > int part; > > > - if (desc->part_type != PART_TYPE_ISO) > > > - return 0; > > > - > > > - /* and devices for each partition: */ > > > + /* Add devices for each partition */ > > > for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > > > if (part_get_info(desc, part, &info)) > > > continue; > > > @@ -289,10 +285,6 @@ static int efi_disk_create_eltorito(struct blk_desc *desc, > > > disks++; > > > } > > > - /* ... and add block device: */ > > > - efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0); > > > -#endif > > > - > > > return disks; > > > } > > > @@ -318,31 +310,18 @@ int efi_disk_register(void) > > > uclass_next_device_check(&dev)) { > > > struct blk_desc *desc = dev_get_uclass_platdata(dev); > > > const char *if_typename = dev->driver->name; > > > - disk_partition_t info; > > > - int part; > > > printf("Scanning disk %s...\n", dev->name); > > > - /* add devices for each partition: */ > > > - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > > > - if (part_get_info(desc, part, &info)) > > > - continue; > > > - efi_disk_add_dev(dev->name, if_typename, desc, > > > - desc->devnum, 0, part); > > > - } > > > - > > > - /* ... and add block device: */ > > > + /* Add block device for the full device */ > > > efi_disk_add_dev(dev->name, if_typename, desc, > > > desc->devnum, 0, 0); > > > disks++; > > > - /* > > > - * El Torito images show up as block devices in an EFI world, > > > - * so let's create them here > > > - */ > > > - disks += efi_disk_create_eltorito(desc, if_typename, > > > - desc->devnum, dev->name); > > > + /* Partitions show up as block devices in EFI */ > > > + disks += efi_disk_create_partitions(desc, if_typename, > > > + desc->devnum, dev->name); > > > } > > > #else > > > int i, if_type; > > > @@ -361,8 +340,6 @@ int efi_disk_register(void) > > > for (i = 0; i < 4; i++) { > > > struct blk_desc *desc; > > > char devname[32] = { 0 }; /* dp->str is u16[32] long */ > > > - disk_partition_t info; > > > - int part; > > > desc = blk_get_devnum_by_type(if_type, i); > > > if (!desc) > > > @@ -373,24 +350,13 @@ int efi_disk_register(void) > > > snprintf(devname, sizeof(devname), "%s%d", > > > if_typename, i); > > > - /* add devices for each partition: */ > > > - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { > > > - if (part_get_info(desc, part, &info)) > > > - continue; > > > - efi_disk_add_dev(devname, if_typename, desc, > > > - i, 0, part); > > > - } > > > - > > > - /* ... and add block device: */ > > > + /* Add block device for the full device */ > > > efi_disk_add_dev(devname, if_typename, desc, i, 0, 0); > > > disks++; > > > - /* > > > - * El Torito images show up as block devices > > > - * in an EFI world, so let's create them here > > > - */ > > > - disks += efi_disk_create_eltorito(desc, if_typename, > > > - i, devname); > > > + /* Partitions show up as block devices in EFI */ > > > + disks += efi_disk_create_partitions(desc, if_typename, > > > + i, devname); > > > } > > > } > > > #endif > > > -- > > > 2.12.3 > > > > > > _______________________________________________ > > > U-Boot mailing list > > > U-Boot@lists.denx.de > > > https://lists.denx.de/listinfo/u-boot > > > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
> From: Alexander Graf <agraf@suse.de> > Date: Thu, 7 Dec 2017 11:27:15 +0100 > > On 07.12.17 08:00, Jonathan Gray wrote: > > On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: > >> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried > >> to introduce the el torito scheme to all partition table types: Spawn > >> individual disk objects for each partition on a disk. > >> > >> Unfortunately, that code ended up creating partitions with offset=0 which meant > >> that anyone accessing these objects gets data from the raw block device instead > >> of the partition. > >> > >> Furthermore, all the el torito logic to spawn devices for partitions was > >> duplicated. So let's merge the two code paths and give partition disk objects > >> good offsets to work from, so that payloads can actually make use of them. > >> > >> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) > >> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> > >> Signed-off-by: Alexander Graf <agraf@suse.de> > > > > This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE > > node with the loaded image protocol on rpi_3 with mmc/usb. > > Hooray :). Do you think you could somehow assemble a test that runs > inside Travis-CI to make sure we catch your loader? I still need to do a > better one for grub to ensure that that one's happy too. Yes, I'd very much like to do that. I suppose this should be a qemu-based test in test/py/tests? It is not clear to me how I run those outside of Travis-CI. Can you point me in the right direction?
On Fri, Dec 08, 2017 at 06:55:02AM +0100, Alexander Graf wrote: > > > On 07.12.17 12:45, Jonathan Gray wrote: > > On Thu, Dec 07, 2017 at 11:57:43AM +0100, Heinrich Schuchardt wrote: > >> On 12/07/2017 08:00 AM, Jonathan Gray wrote: > >>> On Fri, Dec 01, 2017 at 04:10:33PM +0100, Alexander Graf wrote: > >>>> Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried > >>>> to introduce the el torito scheme to all partition table types: Spawn > >>>> individual disk objects for each partition on a disk. > >>>> > >>>> Unfortunately, that code ended up creating partitions with offset=0 which meant > >>>> that anyone accessing these objects gets data from the raw block device instead > >>>> of the partition. > >>>> > >>>> Furthermore, all the el torito logic to spawn devices for partitions was > >>>> duplicated. So let's merge the two code paths and give partition disk objects > >>>> good offsets to work from, so that payloads can actually make use of them. > >>>> > >>>> Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) > >>>> Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> > >>>> Signed-off-by: Alexander Graf <agraf@suse.de> > >>> > >>> This once again broke being able to find a DEVICE_PATH_TYPE_MEDIA_DEVICE > >>> node with the loaded image protocol on rpi_3 with mmc/usb. > >> > >> Could you, please, specify which software you are trying to call: > >> Linux EFI stub, Grub, or anything else? > > > > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/BOOTAA64.EFI > > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/stand/efiboot/ > > Disk image with fat/ffs filesystems > > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/arm64/miniroot62.fs > > > > though it would likely show up on other archs as well > > > > armv7 equivalents of the above > > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/BOOTARM.EFI > > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/armv7/stand/efiboot/ > > https://fastly.cdn.openbsd.org/pub/OpenBSD/snapshots/armv7/miniroot-am335x-62.fs > > > >> > >> Which patches did you consider? > >> Did you apply these patch series that are not yet in efi-next? > >> efi_loader: correct media device paths > >> efi_loader: avoid use after free > > > > just master > > c8e1ca3ebfd21915f6f2e399c9ca1cd3d7a4b076 tools: omapimage: fix corner-case in byteswap path > > > > with a small patch to force calling gnu sed for some non-portable > > sed use in check-config.sh > > > > 'efi_loader: avoid use after free' doesn't help > > 'efi_loader: correct media device paths' doesn't either > > As a quick heads-up: The device path matching is broken. The patch below > should fix it, but I want to create a travis-ci case around that first > and also wrap it up more nicely. > > Alex With the latest master at the time of writing (2e87980580d0bf4781ad0d63efd456aa1a73d03f) the rpi_3 target again seems to be broken while it worked with the v2018.01 release. The qemu_arm64 target however works with the latest master. U-Boot 2018.03-rc1-00058-g36d3bd9514 (Feb 01 2018 - 17:04:29 +1100) DRAM: 948 MiB RPI 3 Model B (0xa02082) MMC: mmc@7e202000: 0, sdhci@7e300000: 1 Loading Environment from FAT... OK In: serial Out: vidconsole Err: vidconsole Net: No ethernet found. starting USB... USB0: Core Release: 2.80a scanning bus 0 for devices... 4 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found Hit any key to stop autoboot: 0 Device 0: Vendor: SanDisk Rev: 1.00 Prod: Ultra Type: Removable Hard Disk Capacity: 29327.3 MB = 28.6 GB (60062500 x 512) ... is now current device Scanning usb 0:1... Found EFI removable media binary efi/boot/bootaa64.efi 78335 bytes read in 79 ms (967.8 KiB/s) ## Starting EFI application at 01000000 ... Scanning disk mmc@7e202000.blk... Card did not respond to voltage select! mmc_init: -95, time 24 Scanning disk sdhci@7e300000.blk... >> OpenBSD/arm64 BOOTAA64 0.8 boot> cannot open sd0a:/etc/random.seed: Device not configured booting sd0a:/bsd: open sd0a:/bsd: Device not configured failed(6). will try /bsd boot> cannot open sd0a:/etc/random.seed: Device not configured booting sd0a:/bsd: open sd0a:/bsd: Device not configured failed(6). will try /bsd Turning timeout off. U-Boot 2018.01 (Jan 09 2018 - 16:25:44 +1100) DRAM: 948 MiB RPI 3 Model B (0xa02082) MMC: sdhci@7e300000: 0 reading uboot.env In: serial Out: vidconsole Err: vidconsole Net: No ethernet found. starting USB... USB0: Core Release: 2.80a scanning bus 0 for devices... 4 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found Hit any key to stop autoboot: 0 Device 0: Vendor: SanDisk Rev: 1.00 Prod: Ultra Type: Removable Hard Disk Capacity: 29327.3 MB = 28.6 GB (60062500 x 512) ... is now current device Scanning usb 0:1... Found EFI removable media binary efi/boot/bootaa64.efi reading efi/boot/bootaa64.efi 78335 bytes read in 81 ms (944.3 KiB/s) ## Starting EFI application at 01000000 ... Scanning disk sdhci@7e300000.blk... Scanning disk usb_mass_storage.lun0... Found 6 disks >> OpenBSD/arm64 BOOTAA64 0.8 boot> booting sd0a:/bsd: 3890560+575675+581952+806704|[276788+96+457320+243052]=0x841d20 ... U-Boot 2018.03-rc1-00058-g36d3bd9514 (Feb 01 2018 - 17:05:55 +1100) DRAM: 2 GiB In: pl011@9000000 Out: pl011@9000000 Err: pl011@9000000 Net: No ethernet found. Hit any key to stop autoboot: 0 scanning bus for devices... Target spinup took 0 ms. SATA link 1 timeout. SATA link 2 timeout. SATA link 3 timeout. SATA link 4 timeout. SATA link 5 timeout. AHCI 0001.0000 32 slots 6 ports 1.5 Gbps 0x3f impl SATA mode flags: 64bit ncq only Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+ Type: Hard Disk Capacity: 2048.0 MB = 2.0 GB (4194304 x 512) Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+ Type: Hard Disk Capacity: 2048.0 MB = 2.0 GB (4194304 x 512) ... is now current device Scanning scsi 0:1... load - load binary file from a filesystem Usage: load <interface> [<dev[:part]> [<addr> [<filename> [bytes [pos]]]]] - Load binary file 'filename' from partition 'part' on device type 'interface' instance 'dev' to address 'addr' in memory. 'bytes' gives the size to load in bytes. If 'bytes' is 0 or omitted, the file is read until the end. 'pos' gives the file byte position to start reading from. If 'pos' is 0 or omitted, the file is read from the start. Found EFI removable media binary efi/boot/bootaa64.efi Scanning disk ahci_scsi.id0lun0... Found 3 disks 78335 bytes read in 9 ms (8.3 MiB/s) ## Starting EFI application at 40400000 ... >> OpenBSD/arm64 BOOTAA64 0.8 boot> booting sd0a:/bsd: 3955580+580404+584072+804720|[278621+96+459048+243979]=0x842f30 ...
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 68ba2cf7b2..4e457a841b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -264,21 +264,17 @@ out_of_memory: printf("ERROR: Out of memory\n"); } -static int efi_disk_create_eltorito(struct blk_desc *desc, - const char *if_typename, - int diskid, - const char *pdevname) +static int efi_disk_create_partitions(struct blk_desc *desc, + const char *if_typename, + int diskid, + const char *pdevname) { int disks = 0; -#if CONFIG_IS_ENABLED(ISO_PARTITION) char devname[32] = { 0 }; /* dp->str is u16[32] long */ disk_partition_t info; int part; - if (desc->part_type != PART_TYPE_ISO) - return 0; - - /* and devices for each partition: */ + /* Add devices for each partition */ for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { if (part_get_info(desc, part, &info)) continue; @@ -289,10 +285,6 @@ static int efi_disk_create_eltorito(struct blk_desc *desc, disks++; } - /* ... and add block device: */ - efi_disk_add_dev(devname, if_typename, desc, diskid, 0, 0); -#endif - return disks; } @@ -318,31 +310,18 @@ int efi_disk_register(void) uclass_next_device_check(&dev)) { struct blk_desc *desc = dev_get_uclass_platdata(dev); const char *if_typename = dev->driver->name; - disk_partition_t info; - int part; printf("Scanning disk %s...\n", dev->name); - /* add devices for each partition: */ - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { - if (part_get_info(desc, part, &info)) - continue; - efi_disk_add_dev(dev->name, if_typename, desc, - desc->devnum, 0, part); - } - - /* ... and add block device: */ + /* Add block device for the full device */ efi_disk_add_dev(dev->name, if_typename, desc, desc->devnum, 0, 0); disks++; - /* - * El Torito images show up as block devices in an EFI world, - * so let's create them here - */ - disks += efi_disk_create_eltorito(desc, if_typename, - desc->devnum, dev->name); + /* Partitions show up as block devices in EFI */ + disks += efi_disk_create_partitions(desc, if_typename, + desc->devnum, dev->name); } #else int i, if_type; @@ -361,8 +340,6 @@ int efi_disk_register(void) for (i = 0; i < 4; i++) { struct blk_desc *desc; char devname[32] = { 0 }; /* dp->str is u16[32] long */ - disk_partition_t info; - int part; desc = blk_get_devnum_by_type(if_type, i); if (!desc) @@ -373,24 +350,13 @@ int efi_disk_register(void) snprintf(devname, sizeof(devname), "%s%d", if_typename, i); - /* add devices for each partition: */ - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { - if (part_get_info(desc, part, &info)) - continue; - efi_disk_add_dev(devname, if_typename, desc, - i, 0, part); - } - - /* ... and add block device: */ + /* Add block device for the full device */ efi_disk_add_dev(devname, if_typename, desc, i, 0, 0); disks++; - /* - * El Torito images show up as block devices - * in an EFI world, so let's create them here - */ - disks += efi_disk_create_eltorito(desc, if_typename, - i, devname); + /* Partitions show up as block devices in EFI */ + disks += efi_disk_create_partitions(desc, if_typename, + i, devname); } } #endif
Commit 884bcf6f65 (efi_loader: use proper device-paths for partitions) tried to introduce the el torito scheme to all partition table types: Spawn individual disk objects for each partition on a disk. Unfortunately, that code ended up creating partitions with offset=0 which meant that anyone accessing these objects gets data from the raw block device instead of the partition. Furthermore, all the el torito logic to spawn devices for partitions was duplicated. So let's merge the two code paths and give partition disk objects good offsets to work from, so that payloads can actually make use of them. Fixes: 884bcf6f65 (efi_loader: use proper device-paths for partitions) Reported-by: Yousaf Kaukab <yousaf.kaukab@suse.com> Signed-off-by: Alexander Graf <agraf@suse.de> --- lib/efi_loader/efi_disk.c | 60 ++++++++++------------------------------------- 1 file changed, 13 insertions(+), 47 deletions(-)