| Message ID | 20180629185940.9369-1-semen.protsenko@linaro.org |
|---|---|
| State | Accepted |
| Commit | aa51579f9aecdc509c91d6a9fc9498c2495cf218 |
| Headers | show |
| Series | [v2] cmd: fastboot: Validate user input | expand |
On 29 June 2018 at 11:59, 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")) > > (tested on BeagleBone Black). > > 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> > --- > Changes for v2: > - replace argv check with argc check > - add mentioning of testing platform in commit message > > cmd/fastboot.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) Reviewed-by: Simon Glass <sjg@chromium.org>
Hi Tom, If there is no objections here, can we apply this patch? Thanks. On Sat, Jun 30, 2018 at 7:20 AM, Simon Glass <sjg@chromium.org> wrote: > On 29 June 2018 at 11:59, 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")) >> >> (tested on BeagleBone Black). >> >> 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> >> --- >> Changes for v2: >> - replace argv check with argc check >> - add mentioning of testing platform in commit message >> >> cmd/fastboot.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) > > Reviewed-by: Simon Glass <sjg@chromium.org>
On Sat, Jun 30, 2018 at 7:20 AM, Simon Glass <sjg@chromium.org> wrote: > On 29 June 2018 at 11:59, 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")) >> >> (tested on BeagleBone Black). >> >> 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> >> --- >> Changes for v2: >> - replace argv check with argc check >> - add mentioning of testing platform in commit message >> >> cmd/fastboot.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> Hi Lukasz, Can you please review and merge? Thanks!
Hi Sam, > On Sat, Jun 30, 2018 at 7:20 AM, Simon Glass <sjg@chromium.org> wrote: > > On 29 June 2018 at 11:59, 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")) > >> > >> (tested on BeagleBone Black). > >> > >> 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> > >> --- > >> Changes for v2: > >> - replace argv check with argc check > >> - add mentioning of testing platform in commit message > >> > >> cmd/fastboot.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > Hi Lukasz, > > Can you please review and merge? I've noticed that I was not CC'ed, so I've missed the patch from the mailing list. You may consider using patman for sending patches (which adds recipients automatically). The patch itself seems OK - thanks. Reviewed-by: Lukasz Majewski <lukma@denx.de> I've added it to u-boot-dfu tree. Lets wait for Travis-CI output. > > Thanks! Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Hi Lukasz, On Wed, Jul 25, 2018 at 12:22 AM, Lukasz Majewski <lukma@denx.de> wrote: > > Hi Sam, > > > On Sat, Jun 30, 2018 at 7:20 AM, Simon Glass <sjg@chromium.org> wrote: > > > On 29 June 2018 at 11:59, 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")) > > >> > > >> (tested on BeagleBone Black). > > >> > > >> 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> > > >> --- > > >> Changes for v2: > > >> - replace argv check with argc check > > >> - add mentioning of testing platform in commit message > > >> > > >> cmd/fastboot.c | 13 ++++++++++++- > > >> 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > Hi Lukasz, > > > > Can you please review and merge? > > I've noticed that I was not CC'ed, so I've missed the patch from the > mailing list. > You may consider using patman for sending patches (which adds > recipients automatically). > > > The patch itself seems OK - thanks. > > Reviewed-by: Lukasz Majewski <lukma@denx.de> > > I've added it to u-boot-dfu tree. Lets wait for Travis-CI output. > u-boot-usb was merged into master recently, but this patch is still missing in master. Can you please check that for me, I'm afraid it could have been lost... Thanks! > > > > Thanks! > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
diff --git a/cmd/fastboot.c b/cmd/fastboot.c index e6ae0570d5..ae3a5f627f 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 (argc == 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")) (tested on BeagleBone Black). 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> --- Changes for v2: - replace argv check with argc check - add mentioning of testing platform in commit message cmd/fastboot.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)