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

[Heinrich Schuchardt]

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),
> 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