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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Thu, Mar 11, 2021 at 08:50:22AM +0100, Heinrich Schuchardt wrote:
> On 3/5/21 11:22 PM, Ilias Apalodimas wrote:
> > On the following patches we allow for an initrd path to be stored in
> > Boot#### variables.  Specifically we encode in the FIlePathList[] of
> > the EFI_LOAD_OPTIONS for each Boot#### variable.
> > 
> > The FilePathList[] array looks like this:
> > kernel - 0xff - VenMedia(initrd GUID) - 0x01 - initrd1 - 0x01 initrd2 -0xff
> > So let's add the relevant functions to concatenate and retrieve a device
> > path based on a Vendor GUID.
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   include/efi_loader.h             |  4 ++
> >   lib/efi_loader/efi_device_path.c | 72 ++++++++++++++++++++++++++++++++
> >   2 files changed, 76 insertions(+)
> > 
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index f470bbd636f4..eb11a8c7d4b1 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -738,6 +738,10 @@ struct efi_load_option {
> >   	const u8 *optional_data;
> >   };
> > 
> > +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 *efi_dp_concat(const struct efi_device_path *dp1,
> > +				      const struct efi_device_path *dp2);
> >   efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
> >   					 efi_uintn_t *size);
> >   unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
> > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > index c9315dd45857..cf1321828e87 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -310,6 +310,41 @@ struct efi_device_path *efi_dp_append(const struct efi_device_path *dp1,
> >   	return ret;
> >   }
> > 
> > +/** efi_dp_concat() - Concatenate 2 device paths. The final device path will contain
> > + *                    two device paths separated by and end node (0xff).
> > + *
> > + * @dp1:	First device path
> > + * @size:	Second device path
> > + *
> > + * Return:	concatenated device path or NULL. Caller must free the returned value
> > + */
> > +struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
> > +				      const struct efi_device_path *dp2)
> > +{
> > +	struct efi_device_path *ret;
> > +	efi_uintn_t sz1, sz2;
> > +	void *p;
> > +
> > +	if (!dp1 || !dp2)
> > +		return NULL;
> > +	sz1 = efi_dp_size(dp1);
> > +	sz2 = efi_dp_size(dp2);
> > +	p = dp_alloc(sz1 + sz2 + (2 * sizeof(END)));
> > +	/* both dp1 and dp2 are non-null */
> > +	if (!p)
> > +		return NULL;
> > +	ret = p;
> > +	memcpy(p, dp1, sz1);
> > +	p += sz1;
> > +	memcpy(p, &END, sizeof(END));
> > +	p += sizeof(END);
> > +	memcpy(p, dp2, sz2);
> > +	p += sz2;
> > +	memcpy(p, &END, sizeof(END));
> > +
> > +	return ret;
> > +}
> > +
> >   struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
> >   					   const struct efi_device_path *node)
> >   {
> > @@ -1160,3 +1195,40 @@ ssize_t efi_dp_check_length(const struct efi_device_path *dp,
> >   		dp = (const struct efi_device_path *)((const u8 *)dp + len);
> >   	}
> >   }
> > +
> > +/**
> > + * efi_dp_from_lo() - Get the instance of a Vendor Device Path
> > + *		      in a multi-instance device path that matches
> > + *		      a specific GUID
> 
> The description does make it clear that you are looking for a VenMedia()
> node.
> 
> Please, describe what the function is good for (find the device path for
> an initrd or dtb in a load option).
> 

Ok 

> > + *
> > + * @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(). 

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

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

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

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.


Thanks
/Ilias