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.apalodi...@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
