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