[PATCH 2/6] efi_loader: Add device path related functions for initrd via Boot####

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 11.03.21 10:10, Ilias Apalodimas wrote:
> On Thu, Mar 11, 2021 at 08:50:22AM +0100, Heinrich Schuchardt wrote:
>> On 3/5/21 11:22 PM, Ilias Apalodimas wrote:
>>> On the following patches we allow for an initrd path to be stored in
>>> Boot#### variables.  Specifically we encode in the FIlePathList[] of
>>> the EFI_LOAD_OPTIONS for each Boot#### variable.
>>>
>>> The FilePathList[] array looks like this:
>>> kernel - 0xff - VenMedia(initrd GUID) - 0x01 - initrd1 - 0x01 initrd2 -0xff
>>> So let's add the relevant functions to concatenate and retrieve a device
>>> path based on a Vendor GUID.
>>>
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>>   include/efi_loader.h             |  4 ++
>>>   lib/efi_loader/efi_device_path.c | 72 ++++++++++++++++++++++++++++++++
>>>   2 files changed, 76 insertions(+)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index f470bbd636f4..eb11a8c7d4b1 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -738,6 +738,10 @@ struct efi_load_option {
>>>   	const u8 *optional_data;
>>>   };
>>>
>>> +struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
>>> +				       efi_uintn_t *size, efi_guid_t guid);
>>> +struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
>>> +				      const struct efi_device_path *dp2);
>>>   efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
>>>   					 efi_uintn_t *size);
>>>   unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
>>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>>> index c9315dd45857..cf1321828e87 100644
>>> --- a/lib/efi_loader/efi_device_path.c
>>> +++ b/lib/efi_loader/efi_device_path.c
>>> @@ -310,6 +310,41 @@ struct efi_device_path *efi_dp_append(const struct efi_device_path *dp1,
>>>   	return ret;
>>>   }
>>>
>>> +/** efi_dp_concat() - Concatenate 2 device paths. The final device path will contain
>>> + *                    two device paths separated by and end node (0xff).
>>> + *
>>> + * @dp1:	First device path
>>> + * @size:	Second device path
>>> + *
>>> + * Return:	concatenated device path or NULL. Caller must free the returned value
>>> + */
>>> +struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
>>> +				      const struct efi_device_path *dp2)
>>> +{
>>> +	struct efi_device_path *ret;
>>> +	efi_uintn_t sz1, sz2;
>>> +	void *p;
>>> +
>>> +	if (!dp1 || !dp2)
>>> +		return NULL;
>>> +	sz1 = efi_dp_size(dp1);
>>> +	sz2 = efi_dp_size(dp2);
>>> +	p = dp_alloc(sz1 + sz2 + (2 * sizeof(END)));
>>> +	/* both dp1 and dp2 are non-null */
>>> +	if (!p)
>>> +		return NULL;
>>> +	ret = p;
>>> +	memcpy(p, dp1, sz1);
>>> +	p += sz1;
>>> +	memcpy(p, &END, sizeof(END));
>>> +	p += sizeof(END);
>>> +	memcpy(p, dp2, sz2);
>>> +	p += sz2;
>>> +	memcpy(p, &END, sizeof(END));
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>   struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
>>>   					   const struct efi_device_path *node)
>>>   {
>>> @@ -1160,3 +1195,40 @@ ssize_t efi_dp_check_length(const struct efi_device_path *dp,
>>>   		dp = (const struct efi_device_path *)((const u8 *)dp + len);
>>>   	}
>>>   }
>>> +
>>> +/**
>>> + * efi_dp_from_lo() - Get the instance of a Vendor Device Path
>>> + *		      in a multi-instance device path that matches
>>> + *		      a specific GUID
>>
>> The description does make it clear that you are looking for a VenMedia()
>> node.
>>
>> Please, describe what the function is good for (find the device path for
>> an initrd or dtb in a load option).
>>
>
> Ok
>
>>> + *
>>> + * @load_option: device paths to search
>>> + * @size:	 size of the discovered device path
>>> + * @guid:	 guid to search for
>>> + *
>>> + * Return:	device path or NULL. Caller must free the returned value
>>
>> Please, keep the text aligned.
>>
>> Do we need a copy? Isn't a pointer good enough?
>
> A pointer to what?
> I think it's better to a copy here. This is a device path that might be used
> out of a stack context were the load option might disappear.
> Look at how the function is used in efi_get_dp_from_boot().

You are duplicating in get_initrd_fp(). Why should we duplicate twice?

>
>>
>>> + */
>>> +struct
>>> +efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
>>> +				efi_uintn_t *size, efi_guid_t guid)
>>> +{
>>> +	struct efi_device_path *fp = lo->file_path;
>>> +	struct efi_device_path_vendor *vendor;
>>> +	int lo_len = lo->file_path_length;
>>> +
>>> +	while (lo_len) {
>>
>> lo_len must be at least sizeof(struct efi_device_path).
>>
>>> +		if (fp->type != DEVICE_PATH_TYPE_MEDIA_DEVICE ||
>>> +		    fp->sub_type != DEVICE_PATH_SUB_TYPE_VENDOR_PATH) {
>>> +			lo_len -= fp->length;
>>
>> Could the last device path in the array be followed by zero bytes for
>> padding?
>
> How? Device paths are packed aren't they ?
>
>> Should we check that fp->length >= sizeof(struct efi_device_path)?
>
> Yea probably a good idea

The content of the boot option comes from the user. Just assume that it
can contain malicious content.

We should also check that the identified device-path starting at
VenMedia() ends within fp->length using efi_dp_check_length().

>
>>
>>> +			fp = (void *)fp + fp->length;
>>
>> Please, avoid code duplication.
>>
>> E.g.
>>
>> for (; lo_len >= sizeof(struct efi_device_path);
>>      lo_len -= fp->length, fp = (void *)fp + fp->length) {
>
> I can an switch to that, but I really never liked this format.
> It always seemed way less readable to me for some reason.  Maybe because I
> never got used to it ...

Using "for" is only one option. You could use "goto next;" instead.

>
>>
>>> +			continue;
>>> +		}
>>> +
>>> +		vendor = (struct efi_device_path_vendor *)fp;
>>> +		if (!guidcmp(&vendor->guid, &guid))
>>> +			return efi_dp_dup(fp);
>>
>> Should we strip of the VenMedia() node here?
>
> Why? This is not supposed to get the file path. The function says "get device
> path from load option" and that device includes the VenMedia node.
> It would make more sense for me to strip in efi_get_dp_from_boot() for
> example, if you want a helper function to get the initrd path *only*.

The VenMedia() node is not needed anymore once you have found the entry.

>
> But really it's just one invocation of efi_dp_get_next_instance() after
> whatever device path you get. Which also modifies the device path pointer, so
> I'd really prefer keeping that call in a local context.

Why next instance? I thought next node.

My understanding is that we have:

kernel path,end(0xff),
VenMedia(), /* no end node here */
initrd1, end(0x01),
initrd2, end(0xff)

Please, document the structure.

Best regards

Heinrich