On 11.03.21 12:44, Heinrich Schuchardt wrote:
Am 11. März 2021 12:36:04 MEZ schrieb 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 ...
I am not referring to efidebug.
The user can update EFI variables with any binary content using an EFI binary or OS functions.
E.g. copy a binary file to the efi variables file system in Linux.
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.
As long as you avoid code duplication I am fine.
Best regards
Heinrich
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),
This end node is superfluous.
Best regards
Heinrich
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