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 <[email protected]>
Signed-off-by: Alexander Graf <[email protected]>

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
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot

Reply via email to