| Message ID | 20180626162057.16043-1-semen.protsenko@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cmd: fastboot: Validate user input | expand |
Hi Sam, On 26 June 2018 at 10:20, Sam Protsenko <semen.protsenko@linaro.org> wrote: > In case when user provides '-' as USB controller index, like this: > > => fastboot - > > data abort occurs in strcmp() function in do_fastboot(), here: > > if (!strcmp(argv[1], "udp")) > > That's because argv[1] is NULL when user types in the '-', and null > pointer dereference occurs in strcmp() (which is ok according to C > standard specification). So we must validate user input to prevent such > behavior. > > While at it, check also the result of strtoul() function and handle > error cases properly. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > cmd/fastboot.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) Reviewed-by: Simon Glass <sjg@chromium.org> Question below. > > diff --git a/cmd/fastboot.c b/cmd/fastboot.c > index e6ae0570d5..dab686eef4 100644 > --- a/cmd/fastboot.c > +++ b/cmd/fastboot.c > @@ -38,13 +38,18 @@ static int do_fastboot_usb(int argc, char *const argv[], > #if CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT) > int controller_index; > char *usb_controller; > + char *endp; > int ret; > > if (argc < 2) > return CMD_RET_USAGE; > > usb_controller = argv[1]; > - controller_index = simple_strtoul(usb_controller, NULL, 0); > + controller_index = simple_strtoul(usb_controller, &endp, 0); > + if (*endp != '\0') { > + pr_err("Error: Wrong USB controller index format\n"); > + return CMD_RET_FAILURE; > + } > > ret = board_usb_init(controller_index, USB_INIT_DEVICE); > if (ret) { > @@ -120,6 +125,12 @@ NXTARG: > ; > } > > + /* Handle case when USB controller param is just '-' */ > + if (!argv[1]) { Can you check argc instead? I think that is nicer. > + pr_err("Error: Incorrect USB controller index\n"); > + return CMD_RET_USAGE; > + } > + > fastboot_init((void *)buf_addr, buf_size); > > if (!strcmp(argv[1], "udp")) > -- > 2.17.1 >
Hi Simon, On 27 June 2018 at 02:18, Simon Glass <sjg@chromium.org> wrote: > Hi Sam, > > On 26 June 2018 at 10:20, Sam Protsenko <semen.protsenko@linaro.org> wrote: >> In case when user provides '-' as USB controller index, like this: >> >> => fastboot - >> >> data abort occurs in strcmp() function in do_fastboot(), here: >> >> if (!strcmp(argv[1], "udp")) >> >> That's because argv[1] is NULL when user types in the '-', and null >> pointer dereference occurs in strcmp() (which is ok according to C >> standard specification). So we must validate user input to prevent such >> behavior. >> >> While at it, check also the result of strtoul() function and handle >> error cases properly. >> >> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >> --- >> cmd/fastboot.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > Question below. > >> >> diff --git a/cmd/fastboot.c b/cmd/fastboot.c >> index e6ae0570d5..dab686eef4 100644 >> --- a/cmd/fastboot.c >> +++ b/cmd/fastboot.c >> @@ -38,13 +38,18 @@ static int do_fastboot_usb(int argc, char *const argv[], >> #if CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT) >> int controller_index; >> char *usb_controller; >> + char *endp; >> int ret; >> >> if (argc < 2) >> return CMD_RET_USAGE; >> >> usb_controller = argv[1]; >> - controller_index = simple_strtoul(usb_controller, NULL, 0); >> + controller_index = simple_strtoul(usb_controller, &endp, 0); >> + if (*endp != '\0') { >> + pr_err("Error: Wrong USB controller index format\n"); >> + return CMD_RET_FAILURE; >> + } >> >> ret = board_usb_init(controller_index, USB_INIT_DEVICE); >> if (ret) { >> @@ -120,6 +125,12 @@ NXTARG: >> ; >> } >> >> + /* Handle case when USB controller param is just '-' */ >> + if (!argv[1]) { > > Can you check argc instead? I think that is nicer. > You are right. Code above my changes modifies the argc w.r.t. argv, so we can check if argc == 1, and this would be the error case. I just sent the v2. Thanks for reviewing. >> + pr_err("Error: Incorrect USB controller index\n"); >> + return CMD_RET_USAGE; >> + } >> + >> fastboot_init((void *)buf_addr, buf_size); >> >> if (!strcmp(argv[1], "udp")) >> -- >> 2.17.1 >>
diff --git a/cmd/fastboot.c b/cmd/fastboot.c index e6ae0570d5..dab686eef4 100644 --- a/cmd/fastboot.c +++ b/cmd/fastboot.c @@ -38,13 +38,18 @@ static int do_fastboot_usb(int argc, char *const argv[], #if CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT) int controller_index; char *usb_controller; + char *endp; int ret; if (argc < 2) return CMD_RET_USAGE; usb_controller = argv[1]; - controller_index = simple_strtoul(usb_controller, NULL, 0); + controller_index = simple_strtoul(usb_controller, &endp, 0); + if (*endp != '\0') { + pr_err("Error: Wrong USB controller index format\n"); + return CMD_RET_FAILURE; + } ret = board_usb_init(controller_index, USB_INIT_DEVICE); if (ret) { @@ -120,6 +125,12 @@ NXTARG: ; } + /* Handle case when USB controller param is just '-' */ + if (!argv[1]) { + pr_err("Error: Incorrect USB controller index\n"); + return CMD_RET_USAGE; + } + fastboot_init((void *)buf_addr, buf_size); if (!strcmp(argv[1], "udp"))
In case when user provides '-' as USB controller index, like this: => fastboot - data abort occurs in strcmp() function in do_fastboot(), here: if (!strcmp(argv[1], "udp")) That's because argv[1] is NULL when user types in the '-', and null pointer dereference occurs in strcmp() (which is ok according to C standard specification). So we must validate user input to prevent such behavior. While at it, check also the result of strtoul() function and handle error cases properly. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- cmd/fastboot.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)