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

[Ilias Apalodimas <ilias.apalodimas@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 ...

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

> >
> >>
> >>> +			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