[U-Boot] [PATCH] cmd: fastboot: Validate user input

[Sam Protsenko]

Hi Simon,

On 27 June 2018 at 02:18, Simon Glass <sjg at chromium.org> wrote:
> Hi Sam,
>
> On 26 June 2018 at 10:20, Sam Protsenko <semen.protsenko at 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 at linaro.org>
>> ---
>>  cmd/fastboot.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> Reviewed-by: Simon Glass <sjg at 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
>>