On 11.03.21 10:10, Ilias Apalodimas wrote:
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);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 GUIDThe 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().
You are duplicating in get_initrd_fp(). Why should we duplicate twice?
- */
+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.
We should also check that the identified device-path starting at VenMedia() ends within fp->length using efi_dp_check_length().
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.
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.
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)
Please, document the structure.
Best regards
Heinrich