[U-Boot] [PATCH] efi_loader: Fix partition offsets

Fri Dec 8 07:02:01 UTC 2017

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 at suse.com>
>>>>> Signed-off-by: Alexander Graf <agraf at 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