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

[Heinrich Schuchardt]

On 11.03.21 12:44, Heinrich Schuchardt wrote:
> Am 11. März 2021 12:36:04 MEZ schrieb Ilias Apalodimas <ilias.apalodimas at linaro.org>:
>> Hi Heinrich
>>
>> [...]
>>
>>>>>> + * @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?
>>>
>>
>> That's irrelevant though isn't it?
>> I did that in the efi initrd implementation. If someone else does the
>> DTB in
>> the future and device to use efi_dp_from_lo return directly?
>> I'd much prefer an API (since that function goes into an API-related
>> file for
>> device paths), that's safe and requires the user to free the memory,
>> rather
>> than allowing him to accidentally shoot himself in the foot, keeping in
>> mind
>> it's a single copy on a device path, which roughly adds anything on our
>> boot
>> time.
>>
>>>>
>>>>>
>>>>>> + */
>>>>>> +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.
>>>
>>
>> Yea the user doesn't add the device path directly though. The user adds
>> directories and a file, so the normalization is part of this function,
>> applied randomly and locally on a single input? or the device path
>> creation
>> functions which this code uses? Since we use the pattern in a bunch of
>> places
>> I assumed we did take care of that during the functions that create the
>> device
>> paths. I haven't checked though ...
>
> I am not referring to efidebug.
>
> The user can update EFI variables with any binary content using an EFI binary or OS functions.
>
> E.g. copy a binary file to the efi variables file system in Linux.
>
>>
>>> We should also check that the identified device-path starting at
>>> VenMedia() ends within fp->length using efi_dp_check_length().
>>
>> ok
>>
>>>
>>>>
>>>>>
>>>>>> +			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.
>>>
>>
>> I really don't mind, I can just use what you propose.
>
> As long as you avoid code duplication I am fine.
>
> Best regards
>
> Heinrich
>
>>
>>>>
>>>>>
>>>>>> +			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.
>>>
>>
>> Yea it's not but as I said the name of the function says "get the
>> *stored*
>>from a boot option. Not get the one that suits us.
>> There's another reason for that btw, the initrd related functions use
>> that
>> (specifically get_initrd_fp()), to figure out if the Boot#### variable
>> contains an initrd path or not.
>> If the VenMedia is not present at all, the protocol is not installed
>> allowing
>> the kernel to fallback in it's command line 'initrd=' option.
>> If the VenMedia is there though, we are checking the file path of the
>> initrd
>> and if the file's not found we return an error allowing Bootmgr to
>> fallback.
>>
>> If we 'just' return the initrd path, we'll have to introduce another
>> variable
>> in the function, indicating if the VenMedia is present or not so the
>> rest
>> ofthe codepath can decide what to do.
>>
>>>>
>>>> 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)
>>
>> No, the structure is added in cmd/efidebug.c code.
>> It's created with efi_dp_append_instance() on
>> - const struct efi_initrd_dp id_dp
>> - file path of initrd
>>
>> which will create:
>> kernel path,end(0xff),
>> VenMedia(), end(0x01),

This end node is superfluous.

Best regards

Heinrich

>> initrd1, end(0x01),
>> initrd2, end(0xff)
>>
>> I know I originally proposed the one you have, but it seemed cleaner
>> adding
>> an extra instance between VenMedia and the first initrd.
>>
>>>
>>> Please, document the structure.
>>>
>>
>> Sure
>>
>>> Best regards
>>>
>>> Heinrich
>>
>> Thanks
>> /Ilias
>