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
