[U-Boot] [PATCH v2] cmd: fastboot: Validate user input
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(-)
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"))
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
participants (3)
-
Lukasz Majewski -
Sam Protsenko -
Simon Glass