| Message ID | 20181214094712.9051-1-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] efi_loader: implement GetNextVariableName() | expand |
Alex, Heinrich, Please review this patch set. Thanks, -Takahiro Akashi On Fri, Dec 14, 2018 at 06:47:11PM +0900, AKASHI Takahiro wrote: > The current GetNextVariableName() is a placeholder. > With this patch, it works well as expected. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_variable.c | 116 +++++++++++++++++++++++++++++++++- > 1 file changed, 115 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index 19d9cb865f25..dac2f49aa1cc 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -8,6 +8,9 @@ > #include <malloc.h> > #include <charset.h> > #include <efi_loader.h> > +#include <environment.h> > +#include <search.h> > +#include <uuid.h> > > #define READ_ONLY BIT(31) > > @@ -241,14 +244,125 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, efi_guid_t *vendor, > return EFI_EXIT(EFI_SUCCESS); > } > > +static char *efi_variables_list; > +static char *efi_cur_variable; > + > +static efi_status_t parse_uboot_variable(char *variable, > + efi_uintn_t *variable_name_size, > + u16 *variable_name, > + efi_guid_t *vendor, > + u32 *attribute) > +{ > + char *guid, *name, *end, c; > + unsigned long name_size; > + u16 *p; > + > + guid = strchr(variable, '_'); > + if (!guid) > + return EFI_NOT_FOUND; > + guid++; > + name = strchr(guid, '_'); > + if (!name) > + return EFI_NOT_FOUND; > + name++; > + end = strchr(name, '='); > + if (!end) > + return EFI_NOT_FOUND; > + > + /* FIXME: size is in byte or u16? */ > + name_size = end - name; > + if (*variable_name_size < (name_size + 1)) { > + *variable_name_size = name_size + 1; > + return EFI_BUFFER_TOO_SMALL; > + } > + end++; /* point to value */ > + > + p = variable_name; > + utf8_utf16_strncpy(&p, name, name_size); > + variable_name[name_size] = 0; > + > + c = *(name - 1); > + *(name - 1) = '\0'; /* guid need be null-terminated here */ > + uuid_str_to_bin(guid, (unsigned char *)vendor, UUID_STR_FORMAT_GUID); > + *(name - 1) = c; > + > + parse_attr(end, attribute); > + > + return EFI_SUCCESS; > +} > + > /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */ > efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, > u16 *variable_name, > efi_guid_t *vendor) > { > + char *native_name, *variable; > + ssize_t name_len, list_len; > + char regex[256]; > + char * const regexlist[] = {regex}; > + u32 attribute; > + int i; > + efi_status_t ret; > + > EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, vendor); > > - return EFI_EXIT(EFI_DEVICE_ERROR); > + if (!variable_name_size || !variable_name || !vendor) > + EFI_EXIT(EFI_INVALID_PARAMETER); > + > + if (variable_name[0]) { > + /* check null-terminated string */ > + for (i = 0; i < *variable_name_size; i++) > + if (!variable_name[i]) > + break; > + if (i >= *variable_name_size) > + return EFI_EXIT(EFI_INVALID_PARAMETER); > + > + /* search for the last-returned variable */ > + ret = efi_to_native(&native_name, variable_name, vendor); > + if (ret) > + return EFI_EXIT(ret); > + > + name_len = strlen(native_name); > + for (variable = efi_variables_list; variable && *variable;) { > + if (!strncmp(variable, native_name, name_len) && > + variable[name_len] == '=') > + break; > + > + variable = strchr(variable, '\n'); > + if (variable) > + variable++; > + } > + > + free(native_name); > + if (!(variable && *variable)) > + return EFI_EXIT(EFI_INVALID_PARAMETER); > + > + /* next variable */ > + variable = strchr(variable, '\n'); > + if (variable) > + variable++; > + if (!(variable && *variable)) > + return EFI_EXIT(EFI_NOT_FOUND); > + } else { > + /* new search */ > + free(efi_variables_list); > + efi_variables_list = NULL; > + efi_cur_variable = NULL; > + > + snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*"); > + list_len = hexport_r(&env_htab, '\n', > + H_MATCH_REGEX | H_MATCH_KEY, > + &efi_variables_list, 0, 1, regexlist); > + if (list_len <= 0) > + return EFI_EXIT(EFI_NOT_FOUND); > + > + variable = efi_variables_list; > + } > + > + ret = parse_uboot_variable(variable, variable_name_size, variable_name, > + vendor, &attribute); > + > + return EFI_EXIT(ret); > } > > /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#SetVariable.28.29 */ > -- > 2.19.1 >
On 12/14/18 10:47 AM, AKASHI Takahiro wrote: > The current GetNextVariableName() is a placeholder. > With this patch, it works well as expected. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_variable.c | 116 +++++++++++++++++++++++++++++++++- > 1 file changed, 115 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index 19d9cb865f25..dac2f49aa1cc 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -8,6 +8,9 @@ > #include <malloc.h> > #include <charset.h> > #include <efi_loader.h> > +#include <environment.h> > +#include <search.h> > +#include <uuid.h> > > #define READ_ONLY BIT(31) > > @@ -241,14 +244,125 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, efi_guid_t *vendor, > return EFI_EXIT(EFI_SUCCESS); > } > > +static char *efi_variables_list; > +static char *efi_cur_variable; > + Please, provide a comment describing what this function is meant to do. Describe every parameter Clearly state set variable_name_size is in bytes (cf. EmuGetNextVariableName() in EDK2) This function duplicates some of the code in efi_get_variable. So, please, use it in efi_get_variable too. > +static efi_status_t parse_uboot_variable(char *variable, > + efi_uintn_t *variable_name_size, > + u16 *variable_name, > + efi_guid_t *vendor, > + u32 *attribute) > +{ > + char *guid, *name, *end, c; > + unsigned long name_size; > + u16 *p; > + > + guid = strchr(variable, '_'); > + if (!guid) > + return EFI_NOT_FOUND; > + guid++; > + name = strchr(guid, '_'); > + if (!name) > + return EFI_NOT_FOUND; > + name++; > + end = strchr(name, '='); > + if (!end) > + return EFI_NOT_FOUND; > + > + /* FIXME: size is in byte or u16? */ It is in bytes. See comment above. > + name_size = end - name; > + if (*variable_name_size < (name_size + 1)) { > + *variable_name_size = name_size + 1; > + return EFI_BUFFER_TOO_SMALL; > + } > + end++; /* point to value */ > + > + p = variable_name; > + utf8_utf16_strncpy(&p, name, name_size); > + variable_name[name_size] = 0; > + > + c = *(name - 1); > + *(name - 1) = '\0'; /* guid need be null-terminated here */ > + uuid_str_to_bin(guid, (unsigned char *)vendor, UUID_STR_FORMAT_GUID); > + *(name - 1) = c; > + > + parse_attr(end, attribute); You have to update variable_name_size. > + > + return EFI_SUCCESS; > +} > + > /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */ Please add a description of the function here like we have it in efi_bootefi.c > efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, > u16 *variable_name, > efi_guid_t *vendor) > { > + char *native_name, *variable; > + ssize_t name_len, list_len; > + char regex[256]; > + char * const regexlist[] = {regex}; > + u32 attribute; > + int i; > + efi_status_t ret; > + > EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, vendor); > > - return EFI_EXIT(EFI_DEVICE_ERROR); > + if (!variable_name_size || !variable_name || !vendor) > + EFI_EXIT(EFI_INVALID_PARAMETER); > + > + if (variable_name[0]) { This code partially duplicates code in in efi_get_variable. Please, carve out a common function. > + /* check null-terminated string */ > + for (i = 0; i < *variable_name_size; i++) > + if (!variable_name[i]) > + break; > + if (i >= *variable_name_size) > + return EFI_EXIT(EFI_INVALID_PARAMETER); > + > + /* search for the last-returned variable */ > + ret = efi_to_native(&native_name, variable_name, vendor); > + if (ret) > + return EFI_EXIT(ret); > + > + name_len = strlen(native_name); > + for (variable = efi_variables_list; variable && *variable;) { > + if (!strncmp(variable, native_name, name_len) && > + variable[name_len] == '=') > + break; > + You miss to compare the GUID. Consider the case that the GUID changes between two calls. > + variable = strchr(variable, '\n'); > + if (variable) > + variable++; > + } > + > + free(native_name); > + if (!(variable && *variable)) With less parentheses I can read the logic more easily: if (!variable || !*variable) But that is just a question of taste. Please, consider the case that the variable is not on the list because the variable has already been deleted. > + return EFI_EXIT(EFI_INVALID_PARAMETER); > + > + /* next variable */ > + variable = strchr(variable, '\n'); > + if (variable) > + variable++; > + if (!(variable && *variable)) > + return EFI_EXIT(EFI_NOT_FOUND); > + } else { > + /* new search */ Please, put a comment here explaining that the list of the preceding search is freed here. > + free(efi_variables_list); > + efi_variables_list = NULL; > + efi_cur_variable = NULL; > + > + snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*"); > + list_len = hexport_r(&env_htab, '\n', > + H_MATCH_REGEX | H_MATCH_KEY, > + &efi_variables_list, 0, 1, regexlist); > + if (list_len <= 0) > + return EFI_EXIT(EFI_NOT_FOUND); You miss to compare the vendor GUIDs. Please, assume that variables are deleted or inserted while the caller loops over the variables. > + > + variable = efi_variables_list; > + } > + > + ret = parse_uboot_variable(variable, variable_name_size, variable_name, > + vendor, &attribute); > + > + return EFI_EXIT(ret); > } > > /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#SetVariable.28.29 */ > Thanks a lot for filling this gap in our EFI implementation. Best regards Heinrich
Heinrich, Thank you for your quick review. On Wed, Jan 16, 2019 at 07:43:47PM +0100, Heinrich Schuchardt wrote: > On 12/14/18 10:47 AM, AKASHI Takahiro wrote: > > The current GetNextVariableName() is a placeholder. > > With this patch, it works well as expected. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > lib/efi_loader/efi_variable.c | 116 +++++++++++++++++++++++++++++++++- > > 1 file changed, 115 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > > index 19d9cb865f25..dac2f49aa1cc 100644 > > --- a/lib/efi_loader/efi_variable.c > > +++ b/lib/efi_loader/efi_variable.c > > @@ -8,6 +8,9 @@ > > #include <malloc.h> > > #include <charset.h> > > #include <efi_loader.h> > > +#include <environment.h> > > +#include <search.h> > > +#include <uuid.h> > > > > #define READ_ONLY BIT(31) > > > > @@ -241,14 +244,125 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, efi_guid_t *vendor, > > return EFI_EXIT(EFI_SUCCESS); > > } > > > > +static char *efi_variables_list; > > +static char *efi_cur_variable; > > + > > Please, provide a comment describing what this function is meant to do. I think that the function name describes itself clearly, but OK. > Describe every parameter > > Clearly state set variable_name_size is in bytes (cf. > EmuGetNextVariableName() in EDK2) Right. > This function duplicates some of the code in efi_get_variable. So, > please, use it in efi_get_variable too. Which part of code do you mean? I don't think so. > > +static efi_status_t parse_uboot_variable(char *variable, > > + efi_uintn_t *variable_name_size, > > + u16 *variable_name, > > + efi_guid_t *vendor, > > + u32 *attribute) > > +{ > > > > > + char *guid, *name, *end, c; > > + unsigned long name_size; > > + u16 *p; > > + > > + guid = strchr(variable, '_'); > > + if (!guid) > > + return EFI_NOT_FOUND; > > + guid++; > > + name = strchr(guid, '_'); > > + if (!name) > > + return EFI_NOT_FOUND; > > + name++; > > + end = strchr(name, '='); > > + if (!end) > > + return EFI_NOT_FOUND; > > + > > + /* FIXME: size is in byte or u16? */ > > It is in bytes. See comment above. OK > > + name_size = end - name; > > + if (*variable_name_size < (name_size + 1)) { > > + *variable_name_size = name_size + 1; > > + return EFI_BUFFER_TOO_SMALL; > > + } > > + end++; /* point to value */ > > + > > + p = variable_name; > > + utf8_utf16_strncpy(&p, name, name_size); > > + variable_name[name_size] = 0; > > + > > + c = *(name - 1); > > + *(name - 1) = '\0'; /* guid need be null-terminated here */ > > + uuid_str_to_bin(guid, (unsigned char *)vendor, UUID_STR_FORMAT_GUID); > > + *(name - 1) = c; > > + > > + parse_attr(end, attribute); > > You have to update variable_name_size. Right. > > + > > + return EFI_SUCCESS; > > +} > > + > > /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */ > > Please add a description of the function here like we have it in > efi_bootefi.c OK, but not for efi_get/set_variable() as I didn't touch anything there. > > efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, > > u16 *variable_name, > > efi_guid_t *vendor) > > { > > + char *native_name, *variable; > > + ssize_t name_len, list_len; > > + char regex[256]; > > + char * const regexlist[] = {regex}; > > + u32 attribute; > > + int i; > > + efi_status_t ret; > > + > > EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, vendor); > > > > - return EFI_EXIT(EFI_DEVICE_ERROR); > > + if (!variable_name_size || !variable_name || !vendor) > > + EFI_EXIT(EFI_INVALID_PARAMETER); > > + > > + if (variable_name[0]) { > > This code partially duplicates code in in efi_get_variable. Please, > carve out a common function. Which part of code do you mean? I don't see any duplication. > > + /* check null-terminated string */ > > + for (i = 0; i < *variable_name_size; i++) > > + if (!variable_name[i]) > > + break; > > + if (i >= *variable_name_size) > > + return EFI_EXIT(EFI_INVALID_PARAMETER); > > + > > + /* search for the last-returned variable */ > > + ret = efi_to_native(&native_name, variable_name, vendor); > > + if (ret) > > + return EFI_EXIT(ret); > > + > > + name_len = strlen(native_name); > > + for (variable = efi_variables_list; variable && *variable;) { > > + if (!strncmp(variable, native_name, name_len) && > > + variable[name_len] == '=') > > + break; > > + > > You miss to compare the GUID. No, "native_name" already contains a given guid. > Consider the case that the GUID changes between two calls. UEFI specification, section 8.2, clearly describes; When VariableName is a pointer to a Null character, VendorGuid is ignored. etNextVariableName() cannot be used as a filter to return variable names with a specific GUID. Instead, the entire list of variables must be retrieved, and the caller may act as a filter if it chooses. > > + variable = strchr(variable, '\n'); > > + if (variable) > > + variable++; > > + } > > + > > + free(native_name); > > + if (!(variable && *variable)) > > With less parentheses I can read the logic more easily: > > if (!variable || !*variable) > > But that is just a question of taste. Well, this "if" clause corresponds with a termination condition of the previous "for" clause and checks whether a for loop has been exhausted. So my expression would be better IMO. > Please, consider the case that the variable is not on the list because > the variable has already been deleted. ditto > > > + return EFI_EXIT(EFI_INVALID_PARAMETER); > > + > > + /* next variable */ > > + variable = strchr(variable, '\n'); > > + if (variable) > > + variable++; > > + if (!(variable && *variable)) > > + return EFI_EXIT(EFI_NOT_FOUND); > > + } else { > > + /* new search */ > > Please, put a comment here explaining that the list of the preceding > search is freed here. OK > > + free(efi_variables_list); > > + efi_variables_list = NULL; > > + efi_cur_variable = NULL; > > + > > + snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*"); > > + list_len = hexport_r(&env_htab, '\n', > > + H_MATCH_REGEX | H_MATCH_KEY, > > + &efi_variables_list, 0, 1, regexlist); > > + if (list_len <= 0) > > + return EFI_EXIT(EFI_NOT_FOUND); > > You miss to compare the vendor GUIDs. No. Please see UEFI specification quoted above. Thanks, -Takahiro Akashi > Please, assume that variables are deleted or inserted while the caller > loops over the variables. > > + > > + variable = efi_variables_list; > > + } > > + > > + ret = parse_uboot_variable(variable, variable_name_size, variable_name, > > + vendor, &attribute); > > + > > + return EFI_EXIT(ret); > > } > > > > /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#SetVariable.28.29 */ > > > > Thanks a lot for filling this gap in our EFI implementation. > > Best regards > > Heinrich
On 1/17/19 3:14 AM, AKASHI Takahiro wrote: > Heinrich, > > Thank you for your quick review. > > On Wed, Jan 16, 2019 at 07:43:47PM +0100, Heinrich Schuchardt wrote: >> On 12/14/18 10:47 AM, AKASHI Takahiro wrote: >>> The current GetNextVariableName() is a placeholder. >>> With this patch, it works well as expected. >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> lib/efi_loader/efi_variable.c | 116 +++++++++++++++++++++++++++++++++- >>> 1 file changed, 115 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c >>> index 19d9cb865f25..dac2f49aa1cc 100644 >>> --- a/lib/efi_loader/efi_variable.c >>> +++ b/lib/efi_loader/efi_variable.c >>> @@ -8,6 +8,9 @@ >>> #include <malloc.h> >>> #include <charset.h> >>> #include <efi_loader.h> >>> +#include <environment.h> >>> +#include <search.h> >>> +#include <uuid.h> >>> >>> #define READ_ONLY BIT(31) >>> >>> @@ -241,14 +244,125 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, efi_guid_t *vendor, >>> return EFI_EXIT(EFI_SUCCESS); >>> } >>> >>> +static char *efi_variables_list; >>> +static char *efi_cur_variable; >>> + >> >> Please, provide a comment describing what this function is meant to do. > > I think that the function name describes itself clearly, but OK. > >> Describe every parameter >> >> Clearly state set variable_name_size is in bytes (cf. >> EmuGetNextVariableName() in EDK2) > > Right. > >> This function duplicates some of the code in efi_get_variable. So, >> please, use it in efi_get_variable too. > > Which part of code do you mean? I don't think so. Checking buffer size. Comparing vendor GUID. Extracting an EFI variable from a U-Boot variable. > >>> +static efi_status_t parse_uboot_variable(char *variable, >>> + efi_uintn_t *variable_name_size, >>> + u16 *variable_name, >>> + efi_guid_t *vendor, >>> + u32 *attribute) >>> +{ >> >> >> >>> + char *guid, *name, *end, c; >>> + unsigned long name_size; >>> + u16 *p; >>> + >>> + guid = strchr(variable, '_'); >>> + if (!guid) >>> + return EFI_NOT_FOUND; >>> + guid++; >>> + name = strchr(guid, '_'); >>> + if (!name) >>> + return EFI_NOT_FOUND; >>> + name++; >>> + end = strchr(name, '='); >>> + if (!end) >>> + return EFI_NOT_FOUND; >>> + >>> + /* FIXME: size is in byte or u16? */ >> >> It is in bytes. See comment above. > > OK > >>> + name_size = end - name; >>> + if (*variable_name_size < (name_size + 1)) { >>> + *variable_name_size = name_size + 1; >>> + return EFI_BUFFER_TOO_SMALL; >>> + } >>> + end++; /* point to value */ >>> + >>> + p = variable_name; >>> + utf8_utf16_strncpy(&p, name, name_size); >>> + variable_name[name_size] = 0; >>> + >>> + c = *(name - 1); >>> + *(name - 1) = '\0'; /* guid need be null-terminated here */ >>> + uuid_str_to_bin(guid, (unsigned char *)vendor, UUID_STR_FORMAT_GUID); >>> + *(name - 1) = c; >>> + >>> + parse_attr(end, attribute); >> >> You have to update variable_name_size. > > Right. > >>> + >>> + return EFI_SUCCESS; >>> +} >>> + >>> /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */ >> >> Please add a description of the function here like we have it in >> efi_bootefi.c > > OK, but not for efi_get/set_variable() as I didn't touch anything there. For the other functions we should do it in a separate patch. > >>> efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, >>> u16 *variable_name, >>> efi_guid_t *vendor) >>> { >>> + char *native_name, *variable; >>> + ssize_t name_len, list_len; >>> + char regex[256]; >>> + char * const regexlist[] = {regex}; >>> + u32 attribute; >>> + int i; >>> + efi_status_t ret; >>> + >>> EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, vendor); >>> >>> - return EFI_EXIT(EFI_DEVICE_ERROR); >>> + if (!variable_name_size || !variable_name || !vendor) >>> + EFI_EXIT(EFI_INVALID_PARAMETER); >>> + >>> + if (variable_name[0]) { >> >> This code partially duplicates code in in efi_get_variable. Please, >> carve out a common function. > > Which part of code do you mean? I don't see any duplication. > >>> + /* check null-terminated string */ >>> + for (i = 0; i < *variable_name_size; i++) >>> + if (!variable_name[i]) >>> + break; >>> + if (i >= *variable_name_size) >>> + return EFI_EXIT(EFI_INVALID_PARAMETER); >>> + >>> + /* search for the last-returned variable */ >>> + ret = efi_to_native(&native_name, variable_name, vendor); >>> + if (ret) >>> + return EFI_EXIT(ret); >>> + >>> + name_len = strlen(native_name); >>> + for (variable = efi_variables_list; variable && *variable;) { >>> + if (!strncmp(variable, native_name, name_len) && >>> + variable[name_len] == '=') >>> + break; >>> + >> >> You miss to compare the GUID. > > No, "native_name" already contains a given guid. > >> Consider the case that the GUID changes between two calls. > > UEFI specification, section 8.2, clearly describes; > When VariableName is a pointer to a Null character, VendorGuid is ignored. > etNextVariableName() cannot be used as a filter to return variable names > with a specific GUID. Instead, the entire list of variables must be > retrieved, and the caller may act as a filter if it chooses. > Look at the list of error codes. EFI_INVALID_PARAMETER has to be returned if name or GUID does not match an existing variable. Regards Heinrich > >>> + variable = strchr(variable, '\n'); >>> + if (variable) >>> + variable++; >>> + } >>> + >>> + free(native_name); >>> + if (!(variable && *variable)) >> >> With less parentheses I can read the logic more easily: >> >> if (!variable || !*variable) >> >> But that is just a question of taste. > > Well, this "if" clause corresponds with a termination condition of > the previous "for" clause and checks whether a for loop has been exhausted. > So my expression would be better IMO. > >> Please, consider the case that the variable is not on the list because >> the variable has already been deleted. > > ditto > >> >>> + return EFI_EXIT(EFI_INVALID_PARAMETER); >>> + >>> + /* next variable */ >>> + variable = strchr(variable, '\n'); >>> + if (variable) >>> + variable++; >>> + if (!(variable && *variable)) >>> + return EFI_EXIT(EFI_NOT_FOUND); >>> + } else { >>> + /* new search */ >> >> Please, put a comment here explaining that the list of the preceding >> search is freed here. > > OK > >>> + free(efi_variables_list); >>> + efi_variables_list = NULL; >>> + efi_cur_variable = NULL; >>> + >>> + snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*"); >>> + list_len = hexport_r(&env_htab, '\n', >>> + H_MATCH_REGEX | H_MATCH_KEY, >>> + &efi_variables_list, 0, 1, regexlist); >>> + if (list_len <= 0) >>> + return EFI_EXIT(EFI_NOT_FOUND); >> >> You miss to compare the vendor GUIDs. > > No. Please see UEFI specification quoted above. > > Thanks, > -Takahiro Akashi > >> Please, assume that variables are deleted or inserted while the caller >> loops over the variables. >>> + >>> + variable = efi_variables_list; >>> + } >>> + >>> + ret = parse_uboot_variable(variable, variable_name_size, variable_name, >>> + vendor, &attribute); >>> + >>> + return EFI_EXIT(ret); >>> } >>> >>> /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#SetVariable.28.29 */ >>> >> >> Thanks a lot for filling this gap in our EFI implementation. >> >> Best regards >> >> Heinrich >
On Thu, Jan 17, 2019 at 08:09:57AM +0100, Heinrich Schuchardt wrote: > On 1/17/19 3:14 AM, AKASHI Takahiro wrote: > > Heinrich, > > > > Thank you for your quick review. > > > > On Wed, Jan 16, 2019 at 07:43:47PM +0100, Heinrich Schuchardt wrote: > >> On 12/14/18 10:47 AM, AKASHI Takahiro wrote: > >>> The current GetNextVariableName() is a placeholder. > >>> With this patch, it works well as expected. > >>> > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> --- > >>> lib/efi_loader/efi_variable.c | 116 +++++++++++++++++++++++++++++++++- > >>> 1 file changed, 115 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > >>> index 19d9cb865f25..dac2f49aa1cc 100644 > >>> --- a/lib/efi_loader/efi_variable.c > >>> +++ b/lib/efi_loader/efi_variable.c > >>> @@ -8,6 +8,9 @@ > >>> #include <malloc.h> > >>> #include <charset.h> > >>> #include <efi_loader.h> > >>> +#include <environment.h> > >>> +#include <search.h> > >>> +#include <uuid.h> > >>> > >>> #define READ_ONLY BIT(31) > >>> > >>> @@ -241,14 +244,125 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, efi_guid_t *vendor, > >>> return EFI_EXIT(EFI_SUCCESS); > >>> } > >>> > >>> +static char *efi_variables_list; > >>> +static char *efi_cur_variable; > >>> + > >> > >> Please, provide a comment describing what this function is meant to do. > > > > I think that the function name describes itself clearly, but OK. > > > >> Describe every parameter > >> > >> Clearly state set variable_name_size is in bytes (cf. > >> EmuGetNextVariableName() in EDK2) > > > > Right. > > > >> This function duplicates some of the code in efi_get_variable. So, > >> please, use it in efi_get_variable too. > > > > Which part of code do you mean? I don't think so. > > Checking buffer size. > Comparing vendor GUID. > Extracting an EFI variable from a U-Boot variable. Please specify exact line numbers on both side. Otherwise, I don't get you. > > > >>> +static efi_status_t parse_uboot_variable(char *variable, > >>> + efi_uintn_t *variable_name_size, > >>> + u16 *variable_name, > >>> + efi_guid_t *vendor, > >>> + u32 *attribute) > >>> +{ > >> > >> > >> > >>> + char *guid, *name, *end, c; > >>> + unsigned long name_size; > >>> + u16 *p; > >>> + > >>> + guid = strchr(variable, '_'); > >>> + if (!guid) > >>> + return EFI_NOT_FOUND; > >>> + guid++; > >>> + name = strchr(guid, '_'); > >>> + if (!name) > >>> + return EFI_NOT_FOUND; > >>> + name++; > >>> + end = strchr(name, '='); > >>> + if (!end) > >>> + return EFI_NOT_FOUND; > >>> + > >>> + /* FIXME: size is in byte or u16? */ > >> > >> It is in bytes. See comment above. > > > > OK > > > >>> + name_size = end - name; > >>> + if (*variable_name_size < (name_size + 1)) { > >>> + *variable_name_size = name_size + 1; > >>> + return EFI_BUFFER_TOO_SMALL; > >>> + } > >>> + end++; /* point to value */ > >>> + > >>> + p = variable_name; > >>> + utf8_utf16_strncpy(&p, name, name_size); > >>> + variable_name[name_size] = 0; > >>> + > >>> + c = *(name - 1); > >>> + *(name - 1) = '\0'; /* guid need be null-terminated here */ > >>> + uuid_str_to_bin(guid, (unsigned char *)vendor, UUID_STR_FORMAT_GUID); > >>> + *(name - 1) = c; > >>> + > >>> + parse_attr(end, attribute); > >> > >> You have to update variable_name_size. > > > > Right. > > > >>> + > >>> + return EFI_SUCCESS; > >>> +} > >>> + > >>> /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */ > >> > >> Please add a description of the function here like we have it in > >> efi_bootefi.c > > > > OK, but not for efi_get/set_variable() as I didn't touch anything there. > > For the other functions we should do it in a separate patch. Not me. > > > >>> efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, > >>> u16 *variable_name, > >>> efi_guid_t *vendor) > >>> { > >>> + char *native_name, *variable; > >>> + ssize_t name_len, list_len; > >>> + char regex[256]; > >>> + char * const regexlist[] = {regex}; > >>> + u32 attribute; > >>> + int i; > >>> + efi_status_t ret; > >>> + > >>> EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, vendor); > >>> > >>> - return EFI_EXIT(EFI_DEVICE_ERROR); > >>> + if (!variable_name_size || !variable_name || !vendor) > >>> + EFI_EXIT(EFI_INVALID_PARAMETER); > >>> + > >>> + if (variable_name[0]) { > >> > >> This code partially duplicates code in in efi_get_variable. Please, > >> carve out a common function. > > > > Which part of code do you mean? I don't see any duplication. What about this? > > > >>> + /* check null-terminated string */ > >>> + for (i = 0; i < *variable_name_size; i++) > >>> + if (!variable_name[i]) > >>> + break; > >>> + if (i >= *variable_name_size) > >>> + return EFI_EXIT(EFI_INVALID_PARAMETER); > >>> + > >>> + /* search for the last-returned variable */ > >>> + ret = efi_to_native(&native_name, variable_name, vendor); > >>> + if (ret) > >>> + return EFI_EXIT(ret); > >>> + > >>> + name_len = strlen(native_name); > >>> + for (variable = efi_variables_list; variable && *variable;) { > >>> + if (!strncmp(variable, native_name, name_len) && > >>> + variable[name_len] == '=') > >>> + break; > >>> + > >> > >> You miss to compare the GUID. > > > > No, "native_name" already contains a given guid. > > > >> Consider the case that the GUID changes between two calls. > > > > UEFI specification, section 8.2, clearly describes; > > When VariableName is a pointer to a Null character, VendorGuid is ignored. > > etNextVariableName() cannot be used as a filter to return variable names > > with a specific GUID. Instead, the entire list of variables must be > > retrieved, and the caller may act as a filter if it chooses. > > > > Look at the list of error codes. EFI_INVALID_PARAMETER has to be > returned if name or GUID does not match an existing variable. The code below behaves exactly as you mention: /* search for the last-returned variable */ ret = efi_to_native(&native_name, variable_name, vendor); name_len = strlen(native_name); for (variable = efi_variables_list; variable && *variable;) { ... } free(native_name); if (!(variable && *variable)) return EFI_EXIT(EFI_INVALID_PARAMETER); -Takahiro Akashi > Regards > > Heinrich > > > > >>> + variable = strchr(variable, '\n'); > >>> + if (variable) > >>> + variable++; > >>> + } > >>> + > >>> + free(native_name); > >>> + if (!(variable && *variable)) > >> > >> With less parentheses I can read the logic more easily: > >> > >> if (!variable || !*variable) > >> > >> But that is just a question of taste. > > > > Well, this "if" clause corresponds with a termination condition of > > the previous "for" clause and checks whether a for loop has been exhausted. > > So my expression would be better IMO. > > > >> Please, consider the case that the variable is not on the list because > >> the variable has already been deleted. > > > > ditto > > > >> > >>> + return EFI_EXIT(EFI_INVALID_PARAMETER); > >>> + > >>> + /* next variable */ > >>> + variable = strchr(variable, '\n'); > >>> + if (variable) > >>> + variable++; > >>> + if (!(variable && *variable)) > >>> + return EFI_EXIT(EFI_NOT_FOUND); > >>> + } else { > >>> + /* new search */ > >> > >> Please, put a comment here explaining that the list of the preceding > >> search is freed here. > > > > OK > > > >>> + free(efi_variables_list); > >>> + efi_variables_list = NULL; > >>> + efi_cur_variable = NULL; > >>> + > >>> + snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*"); > >>> + list_len = hexport_r(&env_htab, '\n', > >>> + H_MATCH_REGEX | H_MATCH_KEY, > >>> + &efi_variables_list, 0, 1, regexlist); > >>> + if (list_len <= 0) > >>> + return EFI_EXIT(EFI_NOT_FOUND); > >> > >> You miss to compare the vendor GUIDs. > > > > No. Please see UEFI specification quoted above. > > > > Thanks, > > -Takahiro Akashi > > > >> Please, assume that variables are deleted or inserted while the caller > >> loops over the variables. > >>> + > >>> + variable = efi_variables_list; > >>> + } > >>> + > >>> + ret = parse_uboot_variable(variable, variable_name_size, variable_name, > >>> + vendor, &attribute); > >>> + > >>> + return EFI_EXIT(ret); > >>> } > >>> > >>> /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#SetVariable.28.29 */ > >>> > >> > >> Thanks a lot for filling this gap in our EFI implementation. > >> > >> Best regards > >> > >> Heinrich > > >
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 19d9cb865f25..dac2f49aa1cc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -8,6 +8,9 @@ #include <malloc.h> #include <charset.h> #include <efi_loader.h> +#include <environment.h> +#include <search.h> +#include <uuid.h> #define READ_ONLY BIT(31) @@ -241,14 +244,125 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, efi_guid_t *vendor, return EFI_EXIT(EFI_SUCCESS); } +static char *efi_variables_list; +static char *efi_cur_variable; + +static efi_status_t parse_uboot_variable(char *variable, + efi_uintn_t *variable_name_size, + u16 *variable_name, + efi_guid_t *vendor, + u32 *attribute) +{ + char *guid, *name, *end, c; + unsigned long name_size; + u16 *p; + + guid = strchr(variable, '_'); + if (!guid) + return EFI_NOT_FOUND; + guid++; + name = strchr(guid, '_'); + if (!name) + return EFI_NOT_FOUND; + name++; + end = strchr(name, '='); + if (!end) + return EFI_NOT_FOUND; + + /* FIXME: size is in byte or u16? */ + name_size = end - name; + if (*variable_name_size < (name_size + 1)) { + *variable_name_size = name_size + 1; + return EFI_BUFFER_TOO_SMALL; + } + end++; /* point to value */ + + p = variable_name; + utf8_utf16_strncpy(&p, name, name_size); + variable_name[name_size] = 0; + + c = *(name - 1); + *(name - 1) = '\0'; /* guid need be null-terminated here */ + uuid_str_to_bin(guid, (unsigned char *)vendor, UUID_STR_FORMAT_GUID); + *(name - 1) = c; + + parse_attr(end, attribute); + + return EFI_SUCCESS; +} + /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, u16 *variable_name, efi_guid_t *vendor) { + char *native_name, *variable; + ssize_t name_len, list_len; + char regex[256]; + char * const regexlist[] = {regex}; + u32 attribute; + int i; + efi_status_t ret; + EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, vendor); - return EFI_EXIT(EFI_DEVICE_ERROR); + if (!variable_name_size || !variable_name || !vendor) + EFI_EXIT(EFI_INVALID_PARAMETER); + + if (variable_name[0]) { + /* check null-terminated string */ + for (i = 0; i < *variable_name_size; i++) + if (!variable_name[i]) + break; + if (i >= *variable_name_size) + return EFI_EXIT(EFI_INVALID_PARAMETER); + + /* search for the last-returned variable */ + ret = efi_to_native(&native_name, variable_name, vendor); + if (ret) + return EFI_EXIT(ret); + + name_len = strlen(native_name); + for (variable = efi_variables_list; variable && *variable;) { + if (!strncmp(variable, native_name, name_len) && + variable[name_len] == '=') + break; + + variable = strchr(variable, '\n'); + if (variable) + variable++; + } + + free(native_name); + if (!(variable && *variable)) + return EFI_EXIT(EFI_INVALID_PARAMETER); + + /* next variable */ + variable = strchr(variable, '\n'); + if (variable) + variable++; + if (!(variable && *variable)) + return EFI_EXIT(EFI_NOT_FOUND); + } else { + /* new search */ + free(efi_variables_list); + efi_variables_list = NULL; + efi_cur_variable = NULL; + + snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*"); + list_len = hexport_r(&env_htab, '\n', + H_MATCH_REGEX | H_MATCH_KEY, + &efi_variables_list, 0, 1, regexlist); + if (list_len <= 0) + return EFI_EXIT(EFI_NOT_FOUND); + + variable = efi_variables_list; + } + + ret = parse_uboot_variable(variable, variable_name_size, variable_name, + vendor, &attribute); + + return EFI_EXIT(ret); } /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#SetVariable.28.29 */
The current GetNextVariableName() is a placeholder. With this patch, it works well as expected. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_variable.c | 116 +++++++++++++++++++++++++++++++++- 1 file changed, 115 insertions(+), 1 deletion(-)