| Message ID | 20190424063045.14443-4-takahiro.akashi@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: non-volatile variables support | expand |
On 4/24/19 8:30 AM, AKASHI Takahiro wrote: > Efidebug command should be implemented using well-defined EFI interfaces, > rather than using internal functions/data. This change will be needed in > a later patch where UEFI variables are re-implemented. I had trouble to get the point. In the next version I suggest to write: Currently in do_efi_boot_dump() we directly read EFI variables from the related environment variables. To accomodate alternative storage backends we should switch to using the UEFI API instead. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > cmd/efidebug.c | 92 ++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 66 insertions(+), 26 deletions(-) > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > index a40c4f4be286..8890dd7268f1 100644 > --- a/cmd/efidebug.c > +++ b/cmd/efidebug.c > @@ -509,7 +509,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag, > if (argc < 6 || argc > 7) > return CMD_RET_USAGE; > > - id = (int)simple_strtoul(argv[1], &endp, 16); > + id = simple_strtoul(argv[1], &endp, 16); This change is correct but unrelated. Please, put it into a separate patch. Or at least mention it in the commit message. > if (*endp != '\0' || id > 0xffff) > return CMD_RET_USAGE; > > @@ -595,7 +595,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, > > guid = efi_global_variable_guid; > for (i = 1; i < argc; i++, argv++) { > - id = (int)simple_strtoul(argv[1], &endp, 16); > + id = simple_strtoul(argv[1], &endp, 16); Same here. > if (*endp != '\0' || id > 0xffff) > return CMD_RET_FAILURE; > > @@ -693,6 +693,27 @@ static void show_efi_boot_opt(int id) > free(data); > } > > +static bool u16_isxdigit(u16 c) > +{ > + if (c & 0xff00) > + return false; > + > + return isxdigit((u8)c); > +} > + > +static int u16_tohex(u16 c) > +{ > + if (c >= '0' && c < '9') > + return c - '0'; > + if (c >= 'A' && c < 'F') > + return c - 'A' + 10; > + if (c >= 'a' && c < 'f') > + return c - 'a' + 10; Does the UEFI spec really allow lower case hexadecimal numbers here? I only found an example with capital numbers. Would this imply that I could have variables Boot00a0 and Boot00A0 side by side? Which one would be selected by boot option 00a0? Or should SetVariable() make a case insensitive search for variable names? In EDK2 function FindVariableEx() in MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c uses CompareMem() to compare variable names. Function BmCharToUint() is used to check the digits of boot options and has this comment: "It assumes the input Char is in the scope of L'0' ~ L'9' and L'A' ~ L'F'" So let's us assume that variable names are case sensitive and Boot entries can only have capital hexadecimal digits. So u16_isxdigit() is incorrect. > + > + /* dummy */ > + return -1; As we do not check bounds here the function could be reduced to: return c > '9' ? c - ('A' - 0xa) : c - '9'; But I would prefer one library function instead of the two static functions which returns -1 for a non-digit and 0 - 15 for a digit. And a unit test in test/unicoode_ut.c > +} > + > /** > * show_efi_boot_dump() - dump all UEFI load options > * > @@ -709,38 +730,57 @@ static void show_efi_boot_opt(int id) > static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag, > int argc, char * const argv[]) > { > - char regex[256]; > - char * const regexlist[] = {regex}; > - char *variables = NULL, *boot, *value; > - int len; > - int id; > + u16 *var_name16, *p; > + efi_uintn_t buf_size, size; > + efi_guid_t guid; > + int id, i; > + efi_status_t ret; > > if (argc > 1) > return CMD_RET_USAGE; > > - snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+"); > - > - /* TODO: use GetNextVariableName? */ > - len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY, > - &variables, 0, 1, regexlist); > + buf_size = 128; > + var_name16 = malloc(buf_size); > + if (!var_name16) > + return CMD_RET_FAILURE; > > - if (!len) > - return CMD_RET_SUCCESS; > + var_name16[0] = 0; > + for (;;) { > + size = buf_size; > + ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16, > + &guid)); > + if (ret == EFI_NOT_FOUND) > + break; > + if (ret == EFI_BUFFER_TOO_SMALL) { > + buf_size = size; > + p = realloc(var_name16, buf_size); > + if (!p) { > + free(var_name16); > + return CMD_RET_FAILURE; > + } > + var_name16 = p; > + ret = EFI_CALL(efi_get_next_variable_name(&size, > + var_name16, > + &guid)); > + } > + if (ret != EFI_SUCCESS) { > + free(var_name16); > + return CMD_RET_FAILURE; > + } > > - if (len < 0) > - return CMD_RET_FAILURE; > + if (u16_strncmp(var_name16, L"Boot", 4) || var_name16[8] || We can use memcmp() here. This avoids introducing u16_strncmp(). > + !u16_isxdigit(var_name16[4]) || > + !u16_isxdigit(var_name16[5]) || > + !u16_isxdigit(var_name16[6]) || > + !u16_isxdigit(var_name16[7])) > + continue; > > - boot = variables; > - while (*boot) { > - value = strstr(boot, "Boot") + 4; > - id = (int)simple_strtoul(value, NULL, 16); > + for (id = 0, i = 0; i < 4; i++) > + id = (id << 4) + u16_tohex(var_name16[4 + i]); This all can be simplified if we unify u16_isxdigit() and u16_tohex(). Just one function returning -1 if not a hexadecimal and 0 - 15 otherwise. > show_efi_boot_opt(id); > - boot = strchr(boot, '\n'); > - if (!*boot) > - break; > - boot++; > } > - free(variables); > + > + free(var_name16); > > return CMD_RET_SUCCESS; > } > @@ -914,7 +954,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag, > return CMD_RET_FAILURE; > > for (i = 0; i < argc; i++) { > - id = (int)simple_strtoul(argv[i], &endp, 16); > + id = simple_strtoul(argv[i], &endp, 16); This change is correct but unrelated. Please, put it into a separate patch. Or at least mention it in the commit message. Best regards Heinrich > if (*endp != '\0' || id > 0xffff) { > printf("invalid value: %s\n", argv[i]); > ret = CMD_RET_FAILURE; >
On Wed, Apr 24, 2019 at 10:13:37PM +0200, Heinrich Schuchardt wrote: > On 4/24/19 8:30 AM, AKASHI Takahiro wrote: > >Efidebug command should be implemented using well-defined EFI interfaces, > >rather than using internal functions/data. This change will be needed in > >a later patch where UEFI variables are re-implemented. > > I had trouble to get the point. In the next version I suggest to write: > > Currently in do_efi_boot_dump() we directly read EFI variables from the > related environment variables. To accomodate alternative storage > backends we should switch to using the UEFI API instead. Okay. > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >--- > > cmd/efidebug.c | 92 ++++++++++++++++++++++++++++++++++++-------------- > > 1 file changed, 66 insertions(+), 26 deletions(-) > > > >diff --git a/cmd/efidebug.c b/cmd/efidebug.c > >index a40c4f4be286..8890dd7268f1 100644 > >--- a/cmd/efidebug.c > >+++ b/cmd/efidebug.c > >@@ -509,7 +509,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag, > > if (argc < 6 || argc > 7) > > return CMD_RET_USAGE; > > > >- id = (int)simple_strtoul(argv[1], &endp, 16); > >+ id = simple_strtoul(argv[1], &endp, 16); > > This change is correct but unrelated. Please, put it into a separate > patch. Or at least mention it in the commit message. Sure. > > if (*endp != '\0' || id > 0xffff) > > return CMD_RET_USAGE; > > > >@@ -595,7 +595,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, > > > > guid = efi_global_variable_guid; > > for (i = 1; i < argc; i++, argv++) { > >- id = (int)simple_strtoul(argv[1], &endp, 16); > >+ id = simple_strtoul(argv[1], &endp, 16); > > Same here. > > > if (*endp != '\0' || id > 0xffff) > > return CMD_RET_FAILURE; > > > >@@ -693,6 +693,27 @@ static void show_efi_boot_opt(int id) > > free(data); > > } > > > >+static bool u16_isxdigit(u16 c) > >+{ > >+ if (c & 0xff00) > >+ return false; > >+ > >+ return isxdigit((u8)c); > >+} > >+ > >+static int u16_tohex(u16 c) > >+{ > >+ if (c >= '0' && c < '9') > >+ return c - '0'; > >+ if (c >= 'A' && c < 'F') > >+ return c - 'A' + 10; > >+ if (c >= 'a' && c < 'f') > >+ return c - 'a' + 10; > > Does the UEFI spec really allow lower case hexadecimal numbers here? > I only found an example with capital numbers. Would this imply that I > could have variables Boot00a0 and Boot00A0 side by side? Which one would > be selected by boot option 00a0? > > Or should SetVariable() make a case insensitive search for variable names? Good point. I found the description below in UEFI section 3.1.1: Each load option entry resides in a Boot####, Driver####, SysPrep####, OsRecovery#### or PlatformRecovery#### variable where #### is replaced by a unique option number in printable hexadecimal representation using the digits 0-9, and the upper case versions of the characters A-F (0000-FFFF). > In EDK2 function FindVariableEx() in > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > uses CompareMem() to compare variable names. > > Function BmCharToUint() is used to check the digits of boot options and > has this comment: > > "It assumes the input Char is in the scope of L'0' ~ L'9' > and L'A' ~ L'F'" > > So let's us assume that variable names are case sensitive and Boot > entries can only have capital hexadecimal digits. > > So u16_isxdigit() is incorrect. > > >+ > >+ /* dummy */ > >+ return -1; > > As we do not check bounds here the function could be reduced to: > > return c > '9' ? c - ('A' - 0xa) : c - '9'; > > But I would prefer one library function instead of the two static > functions which returns -1 for a non-digit and 0 - 15 for a digit. > And a unit test in test/unicoode_ut.c Okay. > >+} > >+ > > /** > > * show_efi_boot_dump() - dump all UEFI load options > > * > >@@ -709,38 +730,57 @@ static void show_efi_boot_opt(int id) > > static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag, > > int argc, char * const argv[]) > > { > >- char regex[256]; > >- char * const regexlist[] = {regex}; > >- char *variables = NULL, *boot, *value; > >- int len; > >- int id; > >+ u16 *var_name16, *p; > >+ efi_uintn_t buf_size, size; > >+ efi_guid_t guid; > >+ int id, i; > >+ efi_status_t ret; > > > > if (argc > 1) > > return CMD_RET_USAGE; > > > >- snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+"); > >- > >- /* TODO: use GetNextVariableName? */ > >- len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY, > >- &variables, 0, 1, regexlist); > >+ buf_size = 128; > >+ var_name16 = malloc(buf_size); > >+ if (!var_name16) > >+ return CMD_RET_FAILURE; > > > >- if (!len) > >- return CMD_RET_SUCCESS; > >+ var_name16[0] = 0; > >+ for (;;) { > >+ size = buf_size; > >+ ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16, > >+ &guid)); > >+ if (ret == EFI_NOT_FOUND) > >+ break; > >+ if (ret == EFI_BUFFER_TOO_SMALL) { > >+ buf_size = size; > >+ p = realloc(var_name16, buf_size); > >+ if (!p) { > >+ free(var_name16); > >+ return CMD_RET_FAILURE; > >+ } > >+ var_name16 = p; > >+ ret = EFI_CALL(efi_get_next_variable_name(&size, > >+ var_name16, > >+ &guid)); > >+ } > >+ if (ret != EFI_SUCCESS) { > >+ free(var_name16); > >+ return CMD_RET_FAILURE; > >+ } > > > >- if (len < 0) > >- return CMD_RET_FAILURE; > >+ if (u16_strncmp(var_name16, L"Boot", 4) || var_name16[8] || > > We can use memcmp() here. This avoids introducing u16_strncmp(). Okay. > >+ !u16_isxdigit(var_name16[4]) || > >+ !u16_isxdigit(var_name16[5]) || > >+ !u16_isxdigit(var_name16[6]) || > >+ !u16_isxdigit(var_name16[7])) > >+ continue; > > > >- boot = variables; > >- while (*boot) { > >- value = strstr(boot, "Boot") + 4; > >- id = (int)simple_strtoul(value, NULL, 16); > >+ for (id = 0, i = 0; i < 4; i++) > >+ id = (id << 4) + u16_tohex(var_name16[4 + i]); > > This all can be simplified if we unify u16_isxdigit() and u16_tohex(). > Just one function returning -1 if not a hexadecimal and 0 - 15 otherwise. Will manage that. > > show_efi_boot_opt(id); > >- boot = strchr(boot, '\n'); > >- if (!*boot) > >- break; > >- boot++; > > } > >- free(variables); > >+ > >+ free(var_name16); > > > > return CMD_RET_SUCCESS; > > } > >@@ -914,7 +954,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag, > > return CMD_RET_FAILURE; > > > > for (i = 0; i < argc; i++) { > >- id = (int)simple_strtoul(argv[i], &endp, 16); > >+ id = simple_strtoul(argv[i], &endp, 16); > > This change is correct but unrelated. Please, put it into a separate > patch. Or at least mention it in the commit message. Okay Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > if (*endp != '\0' || id > 0xffff) { > > printf("invalid value: %s\n", argv[i]); > > ret = CMD_RET_FAILURE; > > >
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index a40c4f4be286..8890dd7268f1 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -509,7 +509,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag, if (argc < 6 || argc > 7) return CMD_RET_USAGE; - id = (int)simple_strtoul(argv[1], &endp, 16); + id = simple_strtoul(argv[1], &endp, 16); if (*endp != '\0' || id > 0xffff) return CMD_RET_USAGE; @@ -595,7 +595,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, guid = efi_global_variable_guid; for (i = 1; i < argc; i++, argv++) { - id = (int)simple_strtoul(argv[1], &endp, 16); + id = simple_strtoul(argv[1], &endp, 16); if (*endp != '\0' || id > 0xffff) return CMD_RET_FAILURE; @@ -693,6 +693,27 @@ static void show_efi_boot_opt(int id) free(data); } +static bool u16_isxdigit(u16 c) +{ + if (c & 0xff00) + return false; + + return isxdigit((u8)c); +} + +static int u16_tohex(u16 c) +{ + if (c >= '0' && c < '9') + return c - '0'; + if (c >= 'A' && c < 'F') + return c - 'A' + 10; + if (c >= 'a' && c < 'f') + return c - 'a' + 10; + + /* dummy */ + return -1; +} + /** * show_efi_boot_dump() - dump all UEFI load options * @@ -709,38 +730,57 @@ static void show_efi_boot_opt(int id) static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - char regex[256]; - char * const regexlist[] = {regex}; - char *variables = NULL, *boot, *value; - int len; - int id; + u16 *var_name16, *p; + efi_uintn_t buf_size, size; + efi_guid_t guid; + int id, i; + efi_status_t ret; if (argc > 1) return CMD_RET_USAGE; - snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+"); - - /* TODO: use GetNextVariableName? */ - len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY, - &variables, 0, 1, regexlist); + buf_size = 128; + var_name16 = malloc(buf_size); + if (!var_name16) + return CMD_RET_FAILURE; - if (!len) - return CMD_RET_SUCCESS; + var_name16[0] = 0; + for (;;) { + size = buf_size; + ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16, + &guid)); + if (ret == EFI_NOT_FOUND) + break; + if (ret == EFI_BUFFER_TOO_SMALL) { + buf_size = size; + p = realloc(var_name16, buf_size); + if (!p) { + free(var_name16); + return CMD_RET_FAILURE; + } + var_name16 = p; + ret = EFI_CALL(efi_get_next_variable_name(&size, + var_name16, + &guid)); + } + if (ret != EFI_SUCCESS) { + free(var_name16); + return CMD_RET_FAILURE; + } - if (len < 0) - return CMD_RET_FAILURE; + if (u16_strncmp(var_name16, L"Boot", 4) || var_name16[8] || + !u16_isxdigit(var_name16[4]) || + !u16_isxdigit(var_name16[5]) || + !u16_isxdigit(var_name16[6]) || + !u16_isxdigit(var_name16[7])) + continue; - boot = variables; - while (*boot) { - value = strstr(boot, "Boot") + 4; - id = (int)simple_strtoul(value, NULL, 16); + for (id = 0, i = 0; i < 4; i++) + id = (id << 4) + u16_tohex(var_name16[4 + i]); show_efi_boot_opt(id); - boot = strchr(boot, '\n'); - if (!*boot) - break; - boot++; } - free(variables); + + free(var_name16); return CMD_RET_SUCCESS; } @@ -914,7 +954,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag, return CMD_RET_FAILURE; for (i = 0; i < argc; i++) { - id = (int)simple_strtoul(argv[i], &endp, 16); + id = simple_strtoul(argv[i], &endp, 16); if (*endp != '\0' || id > 0xffff) { printf("invalid value: %s\n", argv[i]); ret = CMD_RET_FAILURE;
Efidebug command should be implemented using well-defined EFI interfaces, rather than using internal functions/data. This change will be needed in a later patch where UEFI variables are re-implemented. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- cmd/efidebug.c | 92 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 26 deletions(-)