| Message ID | 20210305222303.2866284-2-ilias.apalodimas@linaro.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [1/6] efi_selftest: Remove loadfile2 for initrd selftests | expand |
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); > 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; > +} >
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); > > 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). > 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(). > > > + */ > > +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 > > > + 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 ... > > > + 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*. 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. Thanks /Ilias
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); >>> 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). >> > > 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
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) 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
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), > 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
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 >
Hi Heinrich, [...] > >> 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. Why?i It's not defined in any standard (in fact these patches will be used for the USWG proposal), it's making our lives a lot easier during parsing, since you only need to get the next instance for the *entire* device path. On the other hand if you mix and match them, you'll have to skip the first entry with dp->len and then start using code to get the next instance. Similar logic applies to appending the instance obviously. Since you now how have to account for your 'special' first node, which isn't that special to begin with. So why exactly is it superflous? Because you can get the next initrd without having to parse an end of instace? Well that's the case for the initrd's as well then .... On the contrary I think it makes the entire device path look more generic, since each node is separated by an end of instance, no matter what the contents are. Thanks /Ilias > > 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 > > >
On 11.03.21 13:39, Ilias Apalodimas wrote: > Hi Heinrich, > > [...] >>>> 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. > > Why?i It's not defined in any standard (in fact these patches will be used for > the USWG proposal), it's making our lives a lot easier during parsing, since > you only need to get the next instance for the *entire* device path. > On the other hand if you mix and match them, you'll have to skip the first > entry with dp->len and then start using code to get the next instance. Similar > logic applies to appending the instance obviously. Since you now how have to > account for your 'special' first node, which isn't that special to begin with. > > So why exactly is it superflous? Because you can get the next initrd without > having to parse an end of instace? Well that's the case for the initrd's as > well then .... > On the contrary I think it makes the entire device path look more generic, > since each node is separated by an end of instance, no matter what the > contents are. We use the VenMedia() node to identify a multi-part device tree containing one or multiple initrd. An end node does not convey any additional information. The first initrd device path can start directly behind the VenMedia() node. This saves space for non-volatile variables which is limited. Best regards Heinrich > > Thanks > /Ilias > >> >> 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 >>> >>
On Thu, Mar 11, 2021 at 01:44:05PM +0100, Heinrich Schuchardt wrote: > On 11.03.21 13:39, Ilias Apalodimas wrote: > > Hi Heinrich, > > > > [...] > >>>> 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. > > > > Why?i It's not defined in any standard (in fact these patches will be used for > > the USWG proposal), it's making our lives a lot easier during parsing, since > > you only need to get the next instance for the *entire* device path. > > On the other hand if you mix and match them, you'll have to skip the first > > entry with dp->len and then start using code to get the next instance. Similar > > logic applies to appending the instance obviously. Since you now how have to > > account for your 'special' first node, which isn't that special to begin with. > > > > So why exactly is it superflous? Because you can get the next initrd without > > having to parse an end of instace? Well that's the case for the initrd's as > > well then .... > > On the contrary I think it makes the entire device path look more generic, > > since each node is separated by an end of instance, no matter what the > > contents are. > > We use the VenMedia() node to identify a multi-part device tree > containing one or multiple initrd. > > An end node does not convey any additional information. > > The first initrd device path can start directly behind the VenMedia() > node. This saves space for non-volatile variables which is limited. > Again, it can. But we can also remove the end node after each initrd no? Then we'd save even more space... My point is, I can't understand why we should complicate the adding code/parsing code, by adding a special use case. I don't think saving 4bytes is really worth the complexity. Thanks /Ilias > Best regards > > Heinrich > > > > > Thanks > > /Ilias > > > >> > >> 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 > >>> > >> >
> 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. Right, I was mostly thinking SetVariable which still goes through u-boot. Copying an entire file is possiible though I guess Cheers /Ilias
On 3/11/21 2:31 PM, Ilias Apalodimas wrote: >> 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. > > Right, I was mostly thinking SetVariable which still goes through u-boot. > Copying an entire file is possiible though I guess dd if=foo of=/sys/firmware/efi/efivars/Boot0123-8be4df61-93ca-11d2-aa0d-00e098032b8c But Linux makes some consistency checks. Best regards Heinrich
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
Akashi-san, On Fri, Mar 12, 2021 at 11:50:32AM +0900, AKASHI Takahiro wrote: > Ilias, > > I may have missed your past discussions, but any way, It's on the boot-architecture mailing list [1] > > On Thu, Mar 11, 2021 at 01:36:04PM +0200, Ilias Apalodimas wrote: > > > [...] > > > 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)? > 0xff is a subtype of 'end the entire device path', while 0x01 is an 'end of instance of a device path and start a new device path' > 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? It's essentially the same thing. It has an effect on the EFI spec and how you interpret it, but honestly it feels as an implementation detail to me, since none of those are standardized anyway. In fact what you are saying was part of my proposal in the original mail (check proposal 1.) Anyway the difference between the two is that what I coded looks like this: FilePathList[0] -> kernel FilePathList[1] -> initrd1 - initrdn while whe other is FilePathList[0] -> kernel FilePathList[1] -> initrd1 FilePathList[2] -> initrd2 FilePathList[n] -> initrdn If we ever manage to wire in the DTBs in there as well it may look like: FilePathList[0] -> kernel FilePathList[1] -> initrd1 - initrdn FilePathList[2] -> dtb1 - dtbn Vs FilePathList[0] -> kernel FilePathList[1] -> initrd1 FilePathList[2] -> initrd2 FilePathList[3] -> dtb1 FilePathList[n] -> initrdn FilePathList[n+1] -> dtb2 > > -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 [1] https://lists.linaro.org/pipermail/boot-architecture/2021-February/001686.html Cheers /Ilias
On Fri, Mar 12, 2021 at 06:10:29AM +0200, Ilias Apalodimas wrote: > Akashi-san, > > On Fri, Mar 12, 2021 at 11:50:32AM +0900, AKASHI Takahiro wrote: > > Ilias, > > > > I may have missed your past discussions, but any way, > > It's on the boot-architecture mailing list [1] > > > > > On Thu, Mar 11, 2021 at 01:36:04PM +0200, Ilias Apalodimas wrote: > > > > > > [...] > > > > > 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)? > > > > 0xff is a subtype of 'end the entire device path', while 0x01 is an 'end of > instance of a device path and start a new device path' > > > 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? > > It's essentially the same thing. It has an effect on the EFI spec and how you > interpret it, but honestly it feels as an implementation detail to me, since > none of those are standardized anyway. > > In fact what you are saying was part of my proposal in the original mail > (check proposal 1.) > > Anyway the difference between the two is that what I coded looks like this: > FilePathList[0] -> kernel > FilePathList[1] -> initrd1 - initrdn > > while whe other is > FilePathList[0] -> kernel > FilePathList[1] -> initrd1 > FilePathList[2] -> initrd2 > FilePathList[n] -> initrdn > > If we ever manage to wire in the DTBs in there as well it may look like: > > FilePathList[0] -> kernel > FilePathList[1] -> initrd1 - initrdn > FilePathList[2] -> dtb1 - dtbn > > Vs > > FilePathList[0] -> kernel > FilePathList[1] -> initrd1 > FilePathList[2] -> initrd2 > FilePathList[3] -> dtb1 > FilePathList[n] -> initrdn > FilePathList[n+1] -> dtb2 What is the semantics? Which do you want to do? a) boot one of combinations: 1.kernel+initrd1+dtb1, or 2.kernel+initrd2+dtb2 b) boot kernel + (initrd1 + initrd2) + (dtb1 + dtb2) I assume you meant (a). In that case, how can you specify (a-1) or (a-2) at boot time? Is there any clear description about that? (I"m simply asking here.) -Takahiro Akashi > > > > > > -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 > > [1] https://lists.linaro.org/pipermail/boot-architecture/2021-February/001686.html > > Cheers > /Ilias
On Fri, Mar 12, 2021 at 01:32:50PM +0900, AKASHI Takahiro wrote: [...] > > > > > 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)? > > > > > > > 0xff is a subtype of 'end the entire device path', while 0x01 is an 'end of > > instance of a device path and start a new device path' > > > > > 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? > > > > It's essentially the same thing. It has an effect on the EFI spec and how you > > interpret it, but honestly it feels as an implementation detail to me, since > > none of those are standardized anyway. > > > > In fact what you are saying was part of my proposal in the original mail > > (check proposal 1.) > > > > Anyway the difference between the two is that what I coded looks like this: > > FilePathList[0] -> kernel > > FilePathList[1] -> initrd1 - initrdn > > > > while whe other is > > FilePathList[0] -> kernel > > FilePathList[1] -> initrd1 > > FilePathList[2] -> initrd2 > > FilePathList[n] -> initrdn > > > > If we ever manage to wire in the DTBs in there as well it may look like: > > > > FilePathList[0] -> kernel > > FilePathList[1] -> initrd1 - initrdn > > FilePathList[2] -> dtb1 - dtbn > > > > Vs > > > > FilePathList[0] -> kernel > > FilePathList[1] -> initrd1 > > FilePathList[2] -> initrd2 > > FilePathList[3] -> dtb1 > > FilePathList[n] -> initrdn > > FilePathList[n+1] -> dtb2 > > What is the semantics? > Which do you want to do? > a) boot one of combinations: > 1.kernel+initrd1+dtb1, or > 2.kernel+initrd2+dtb2 > b) boot > kernel + (initrd1 + initrd2) + (dtb1 + dtb2) > > I assume you meant (a). > In that case, how can you specify (a-1) or (a-2) at boot time? > > Is there any clear description about that? > (I"m simply asking here.) it's b) if you want different combinations of kernel/initrds (as described in a) you can add another Boot#### variable. So let's assume you got three boot options Boot0000, Boot0001 and Boot0002 Boot0000 efi_load_options: kernel1 + (initrd1 + initrd2) + (dtb1 + dtb2). The bootloader will concat initrd1+initrd2 when the kernel requests it. Similar behavior can be coded for dtb before installing the table. Boot0001: kernel1 + initrd1 + dtb1 Load a kernel with a single initrd and dtb. Boot0002: kernel2 + initrd2 + dtb2 ditto. Hope that's clear now Cheers /Ilias > > -Takahiro Akashi > > > > > > > > > > > -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 > > > > [1] https://lists.linaro.org/pipermail/boot-architecture/2021-February/001686.html > > > > Cheers > > /Ilias
On Fri, Mar 12, 2021 at 06:42:14AM +0200, Ilias Apalodimas wrote: > On Fri, Mar 12, 2021 at 01:32:50PM +0900, AKASHI Takahiro wrote: > [...] > > > > > > 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)? > > > > > > > > > > 0xff is a subtype of 'end the entire device path', while 0x01 is an 'end of > > > instance of a device path and start a new device path' If I correctly remember, you had some discussions about this point that UEFI specification is ambiguous here. Right? > > > > > > > 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? > > > > > > It's essentially the same thing. It has an effect on the EFI spec and how you > > > interpret it, but honestly it feels as an implementation detail to me, since > > > none of those are standardized anyway. > > > > > > In fact what you are saying was part of my proposal in the original mail > > > (check proposal 1.) > > > > > > Anyway the difference between the two is that what I coded looks like this: > > > FilePathList[0] -> kernel > > > FilePathList[1] -> initrd1 - initrdn > > > > > > while whe other is > > > FilePathList[0] -> kernel > > > FilePathList[1] -> initrd1 > > > FilePathList[2] -> initrd2 > > > FilePathList[n] -> initrdn > > > > > > If we ever manage to wire in the DTBs in there as well it may look like: > > > > > > FilePathList[0] -> kernel > > > FilePathList[1] -> initrd1 - initrdn > > > FilePathList[2] -> dtb1 - dtbn > > > > > > Vs > > > > > > FilePathList[0] -> kernel > > > FilePathList[1] -> initrd1 > > > FilePathList[2] -> initrd2 > > > FilePathList[3] -> dtb1 > > > FilePathList[n] -> initrdn > > > FilePathList[n+1] -> dtb2 > > > > What is the semantics? > > Which do you want to do? > > a) boot one of combinations: > > 1.kernel+initrd1+dtb1, or > > 2.kernel+initrd2+dtb2 > > b) boot > > kernel + (initrd1 + initrd2) + (dtb1 + dtb2) > > > > I assume you meant (a). > > In that case, how can you specify (a-1) or (a-2) at boot time? > > > > Is there any clear description about that? > > (I"m simply asking here.) > > it's b) OK. > if you want different combinations of kernel/initrds (as described in a) you > can add another Boot#### variable. Indeed. > So let's assume you got three boot options > Boot0000, Boot0001 and Boot0002 > > Boot0000 efi_load_options: kernel1 + (initrd1 + initrd2) + (dtb1 + dtb2). > The bootloader will concat initrd1+initrd2 when the kernel requests it. > Similar behavior can be coded for dtb before installing the table. My understanding is that none of existing boot loaders has not yet supported such a concatenation (of either initrd or dtb) right now. Do you (or linux's efistub?) intend to add a new feature? -Takahiro Akashi > Boot0001: kernel1 + initrd1 + dtb1 > Load a kernel with a single initrd and dtb. > > Boot0002: kernel2 + initrd2 + dtb2 > ditto. > > Hope that's clear now > > Cheers > /Ilias > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > -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 > > > > > > [1] https://lists.linaro.org/pipermail/boot-architecture/2021-February/001686.html > > > > > > Cheers > > > /Ilias
> > > > > > [...] > > > > > > 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)? > > > > > > > > > > > > > 0xff is a subtype of 'end the entire device path', while 0x01 is an 'end of > > > > instance of a device path and start a new device path' > > If I correctly remember, you had some discussions about this point > that UEFI specification is ambiguous here. Right? Yea, just by reading it wasn't clear if the delimiter between FilePathList[] array elements was supposed to be 0xff or 0x01. We finally figured out the spec means 0xff though, so that's (hopefully) clear now. > [...] > > > > FilePathList[0] -> kernel > > > > FilePathList[1] -> initrd1 - initrdn > > > > FilePathList[2] -> dtb1 - dtbn > > > > > > > > Vs > > > > > > > > FilePathList[0] -> kernel > > > > FilePathList[1] -> initrd1 > > > > FilePathList[2] -> initrd2 > > > > FilePathList[3] -> dtb1 > > > > FilePathList[n] -> initrdn > > > > FilePathList[n+1] -> dtb2 > > > > > > What is the semantics? > > > Which do you want to do? > > > a) boot one of combinations: > > > 1.kernel+initrd1+dtb1, or > > > 2.kernel+initrd2+dtb2 > > > b) boot > > > kernel + (initrd1 + initrd2) + (dtb1 + dtb2) > > > > > > I assume you meant (a). > > > In that case, how can you specify (a-1) or (a-2) at boot time? > > > > > > Is there any clear description about that? > > > (I"m simply asking here.) > > > > it's b) > > OK. > > > if you want different combinations of kernel/initrds (as described in a) you > > can add another Boot#### variable. > > Indeed. > > > So let's assume you got three boot options > > Boot0000, Boot0001 and Boot0002 > > > > Boot0000 efi_load_options: kernel1 + (initrd1 + initrd2) + (dtb1 + dtb2). > > The bootloader will concat initrd1+initrd2 when the kernel requests it. > > Similar behavior can be coded for dtb before installing the table. > > My understanding is that none of existing boot loaders has not yet > supported such a concatenation (of either initrd or dtb) right now. > Do you (or linux's efistub?) intend to add a new feature? For u-boot we intend to add it in the future. I just wanted to keep the initial patchset simpler, since 99.9% of the use cases are for a single initrd. The efistub doesn't have to add anything to support this, that's the whole point of trying to standardize this. Since there's some confusion around let me try and explain that here as well. The EFI stub does two things. It searches for the protocol with a specific DP (which includes a unique GUID). The DP our case is <VenMedia for initrd>/<end node>. Since locate protocol will strip the first part, the stub now calls LoadFile2 protocol with a device end node and a kernel provided buffer (and there's a mechanism to discover the size). It's pretty much tells the bootloader 'gimme my initrd'. So it's entirely up to the bootloader to select a mechanism for discovering were those initrds are and concatenate them Cheers /Ilias > > -Takahiro Akashi > > > > Boot0001: kernel1 + initrd1 + dtb1 > > Load a kernel with a single initrd and dtb. > > > > Boot0002: kernel2 + initrd2 + dtb2 > > ditto. > > > > Hope that's clear now > > > > Cheers > > /Ilias > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > > > > > > -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 > > > > > > > > [1] https://lists.linaro.org/pipermail/boot-architecture/2021-February/001686.html > > > > > > > > Cheers > > > > /Ilias
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 + * + * @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 + */ +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) { + if (fp->type != DEVICE_PATH_TYPE_MEDIA_DEVICE || + fp->sub_type != DEVICE_PATH_SUB_TYPE_VENDOR_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); + lo_len -= fp->length; + fp = (void *)fp + fp->length; + } + + return NULL; +}
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(+)