| Message ID | 20200228000504.1814-1-takahiro.akashi@linaro.org |
|---|---|
| State | Accepted, archived |
| Commit | e8bced62b6be344dbc922d88b1b278e316e1585f |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | cmd: efidebug: fix a failure of "boot rm" sub-command | expand |
On 2/28/20 1:05 AM, AKASHI Takahiro wrote: > There is a wrong usage of utf8_utf16_strncpy() in "boot rm" command, and > then it will end up with a failure of this command due to a wrong > value of an interim variable ("var_name16"). > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > cmd/efidebug.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > index 576e95b395dc..3a50dafbbca6 100644 > --- a/cmd/efidebug.c > +++ b/cmd/efidebug.c > @@ -641,7 +641,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, > int id, i; > char *endp; > char var_name[9]; > - u16 var_name16[9]; > + u16 var_name16[9], *p; > efi_status_t ret; > > if (argc == 1) > @@ -654,7 +654,8 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, > return CMD_RET_FAILURE; > > sprintf(var_name, "Boot%04X", id); > - utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9); > + p = var_name16; > + utf8_utf16_strncpy(&p, var_name, 9); This is duplicating code in do_efi_boot_add(). Please, consider putting the following codeblock into separate function: id = (int)simple_strtoul(argv[1], &endp, 16); if (*endp != '\0' || id > 0xffff) return CMD_RET_USAGE; sprintf(var_name, "Boot%04X", id); p = var_name16; utf8_utf16_strncpy(&p, var_name, 9); try_load_entry() has another implementation. Best regards Heinrich > > ret = EFI_CALL(RT->set_variable(var_name16, &guid, 0, 0, NULL)); > if (ret) { >
On Fri, Feb 28, 2020 at 07:05:39PM +0100, Heinrich Schuchardt wrote: > On 2/28/20 1:05 AM, AKASHI Takahiro wrote: > > There is a wrong usage of utf8_utf16_strncpy() in "boot rm" command, and > > then it will end up with a failure of this command due to a wrong > > value of an interim variable ("var_name16"). > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > cmd/efidebug.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > > index 576e95b395dc..3a50dafbbca6 100644 > > --- a/cmd/efidebug.c > > +++ b/cmd/efidebug.c > > @@ -641,7 +641,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, > > int id, i; > > char *endp; > > char var_name[9]; > > - u16 var_name16[9]; > > + u16 var_name16[9], *p; > > efi_status_t ret; > > > > if (argc == 1) > > @@ -654,7 +654,8 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, > > return CMD_RET_FAILURE; > > > > sprintf(var_name, "Boot%04X", id); > > - utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9); > > + p = var_name16; > > + utf8_utf16_strncpy(&p, var_name, 9); > > This is duplicating code in do_efi_boot_add(). Please, consider putting > the following codeblock into separate function: No. > id = (int)simple_strtoul(argv[1], &endp, 16); > if (*endp != '\0' || id > 0xffff) > return CMD_RET_USAGE; > > sprintf(var_name, "Boot%04X", id); > p = var_name16; > utf8_utf16_strncpy(&p, var_name, 9); Frankly, I don't like this function's prototype because it is different from "normal" corresponding C libraries. Moreover, as far as I notice, there is no use case where the first parameter which is modified after calling is used in any code. Thanks, -Takahiro Akashi > try_load_entry() has another implementation. > > Best regards > > Heinrich > > > > > ret = EFI_CALL(RT->set_variable(var_name16, &guid, 0, 0, NULL)); > > if (ret) { > > >
On 3/2/20 1:05 AM, AKASHI Takahiro wrote: > On Fri, Feb 28, 2020 at 07:05:39PM +0100, Heinrich Schuchardt wrote: >> On 2/28/20 1:05 AM, AKASHI Takahiro wrote: >>> There is a wrong usage of utf8_utf16_strncpy() in "boot rm" command, and >>> then it will end up with a failure of this command due to a wrong >>> value of an interim variable ("var_name16"). >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> cmd/efidebug.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c >>> index 576e95b395dc..3a50dafbbca6 100644 >>> --- a/cmd/efidebug.c >>> +++ b/cmd/efidebug.c >>> @@ -641,7 +641,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, >>> int id, i; >>> char *endp; >>> char var_name[9]; >>> - u16 var_name16[9]; >>> + u16 var_name16[9], *p; >>> efi_status_t ret; >>> >>> if (argc == 1) >>> @@ -654,7 +654,8 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, >>> return CMD_RET_FAILURE; >>> >>> sprintf(var_name, "Boot%04X", id); >>> - utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9); >>> + p = var_name16; >>> + utf8_utf16_strncpy(&p, var_name, 9); >> >> This is duplicating code in do_efi_boot_add(). Please, consider putting >> the following codeblock into separate function: > > No. Factoring out a separate function does not reduce the code size. Hence Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >> id = (int)simple_strtoul(argv[1], &endp, 16); >> if (*endp != '\0' || id > 0xffff) >> return CMD_RET_USAGE; >> >> sprintf(var_name, "Boot%04X", id); >> p = var_name16; >> utf8_utf16_strncpy(&p, var_name, 9); > > Frankly, I don't like this function's prototype because it is different > from "normal" corresponding C libraries. > Moreover, as far as I notice, there is no use case where the first > parameter which is modified after calling is used in any code. > > Thanks, > -Takahiro Akashi > >> try_load_entry() has another implementation. >> >> Best regards >> >> Heinrich >> >>> >>> ret = EFI_CALL(RT->set_variable(var_name16, &guid, 0, 0, NULL)); >>> if (ret) { >>> >>
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 576e95b395dc..3a50dafbbca6 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -641,7 +641,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, int id, i; char *endp; char var_name[9]; - u16 var_name16[9]; + u16 var_name16[9], *p; efi_status_t ret; if (argc == 1) @@ -654,7 +654,8 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag, return CMD_RET_FAILURE; sprintf(var_name, "Boot%04X", id); - utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9); + p = var_name16; + utf8_utf16_strncpy(&p, var_name, 9); ret = EFI_CALL(RT->set_variable(var_name16, &guid, 0, 0, NULL)); if (ret) {
There is a wrong usage of utf8_utf16_strncpy() in "boot rm" command, and then it will end up with a failure of this command due to a wrong value of an interim variable ("var_name16"). Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- cmd/efidebug.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)