Skip to contentSkip to conversation

cmd: fastboot: Validate user input

PATCH Superseded Sam Protsenko 3 participants 6 messages Jun 29, 2018 – Aug 13, 2018

lore
Share & tools
lore

Sam Protsenko

author
#0 lore raw

PATCH v2 1/1 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 <[email protected]>

---
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(-)
cmd/fastboot.c +12 -1
index e6ae0570d5..ae3a5f627f 100644
@@ -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"))
Signature · 1 line
2.17.1
#1 lore raw

PATCH v2 1/1 cmd: fastboot: Validate user input

Quoted 18 lines · Sam Protsenko

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.

Quoted 8 lines
Signed-off-by Sam Protsenko <[email protected]>

---
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 <[email protected]>
author
#2 lore raw

PATCH v2 1/1 cmd: fastboot: Validate user input

Hi Tom,

If there is no objections here, can we apply this patch?

Thanks.

Quoted 29 lines · Simon Glass
Quoted 18 lines · Sam Protsenko

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.

Quoted 8 lines
Signed-off-by Sam Protsenko <[email protected]>

---
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 <[email protected]>
author
#3 lore raw

PATCH v2 1/1 cmd: fastboot: Validate user input

In reply to Simon Glass
Quoted 25 lines · Simon Glass
Quoted 24 lines · Sam Protsenko

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 <[email protected]>

---
Changes for v2:
- replace argv check with argc check
- add mentioning of testing platform in commit message

Quoted 4 lines
Quoted 2 lines
 cmd/fastboot.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Reviewed-by Simon Glass <[email protected]>

Hi Lukasz,

Can you please review and merge?

Thanks!

#4 lore raw

PATCH v2 1/1 cmd: fastboot: Validate user input

Hi Sam,

Quoted 27 lines · Simon Glass
Quoted 26 lines
Quoted 24 lines · Sam Protsenko

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 <[email protected]>

---
Changes for v2:
- replace argv check with argc check
- add mentioning of testing platform in commit message

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
 cmd/fastboot.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Reviewed-by Simon Glass <[email protected]>

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 <[email protected]>

I've added it to u-boot-dfu tree. Lets wait for Travis-CI output.

Quoted 2 lines

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 at denx.de -------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180724/a154b52f/attachment.sig>

author
#5 lore raw

PATCH v2 1/1 cmd: fastboot: Validate user input

Hi Lukasz,

Quoted 45 lines · Lukasz Majewski

Hi Sam,

Quoted 27 lines · Simon Glass
Quoted 26 lines
Quoted 24 lines · Sam Protsenko

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 <[email protected]>

---
Changes for v2:
- replace argv check with argc check
- add mentioning of testing platform in commit message

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
 cmd/fastboot.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Reviewed-by Simon Glass <[email protected]>

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).

Quoted 6 lines

The patch itself seems OK - thanks.

Reviewed-by Lukasz Majewski <[email protected]>

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!

Quoted 15 lines
Quoted 2 lines

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 at denx.de