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

[AKASHI Takahiro]

Ilias,

I may have missed your past discussions, but any way,

On Thu, Mar 11, 2021 at 01:36:04PM +0200, Ilias Apalodimas wrote:
> 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)

What is the difference between end(0xff) and end(0x01)?

If the first argument of a load option is a list of device paths,
I would expect the format would look like:
  kernel path,end(0xff),
  VenMedia(INITRD),initrd1 path,end(0xff),
  VenMedia(INITRD),initrd2 path,end(0xff),

so that VenMedia can work as an identify of the succeeding path.
Is it simple enough, isn't it?

-Takahiro Akashi

> 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