Am 11. März 2021 12:36:04 MEZ schrieb Ilias Apalodimas
<ilias.apalodi...@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
