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

[Heinrich Schuchardt]

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

> + *
> + * @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?

> + */
> +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?
Should we check that fp->length >= sizeof(struct efi_device_path)?

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

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

Best regards

Heinrich

> +		lo_len -= fp->length;
> +		fp = (void *)fp + fp->length;
> +	}
> +
> +	return NULL;
> +}
>