[PATCH] autoboot: fix illegal memory access when stop key and delay key are empty

[Heinrich Schuchardt]

On 1/18/21 6:38 AM, Andy.Wu at sony.com wrote:
>>> You could avoid the subtraction instead of changing the type:
>>>
>>> -for (i = 0; i < presskey_max - 1; i++)
>>> +for (i = 0; i + 1 < presskey_max; i++)
>> This style seems not typically way for for loop, how do you think?
> I found one similar for loop style in u-boot source code, it seems aim to fix the similar issue.
>
> $ grep -rn "i = 0; i + 1 < " . | grep for
> drivers/i2c/meson_i2c.c:132:    for (i = 0; i + 1 < i2c->count; i++)
>
> This for loop style just 1 place compared with common style 3926 in u-boot.
>
> $ grep -rn "i = 0; i + 1 < " . | grep for | wc -l
> 1
> $ grep -rn "i = 0; i < " . | grep for | wc -l
> 3926
>
> Best Regards
> Andy Wu
>
>> -----Original Message-----
>> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of
>> Andy.Wu at sony.com
>> Sent: Monday, January 18, 2021 1:22 PM
>> To: xypron.glpk at gmx.de; Mo, Yuezhang <Yuezhang.Mo at sony.com>;
>> u-boot at lists.denx.de
>> Cc: sjg at chromium.org; hs at denx.de
>> Subject: RE: [PATCH] autoboot: fix illegal memory access when stop key and
>> delay key are empty
>>
>>> Both indices cannot be negative. So it is understandable that u_int was chosen.
>> Yes, u_int is understandable that the length is never be negative, but define the
>> length parameter as "int" also usable.
>> Some example in current u-boot source code:
>>
>> $ grep -rn length common/ | grep "int "
>> common/usb_storage.c:363:static int us_one_transfer(struct us_data *us, int
>> pipe, char *buf, int length) common/lcd.c:52:int lcd_line_length;
>> common/lcd.c:66:        int line_length;
>> common/lcd.c:143:__weak int lcd_get_size(int *line_length)
>> common/lcd.c:283:       int line_length;
>> common/usb.c:275:                       void *data, int len, int
>> *actual_length, int timeout)
>> common/usb.c:600:                            unsigned char *buffer, int
>> length)
>> common/usb.c:751:static void usb_try_string_workarounds(unsigned char *buf,
>> int *length)
>> common/usb.c:753:       int newlength, oldlength = *length;
>> common/kgdb.c:319:      int length;
>> common/cli_hush.c:318:  int length;
>> common/usb_hub.c:613:   int i, length;
>>
>>> You could avoid the subtraction instead of changing the type:
>>>
>>> -for (i = 0; i < presskey_max - 1; i++)
>>> +for (i = 0; i + 1 < presskey_max; i++)
>> This style seems not typically way for for loop, how do you think?

I found 3 of these:

common/usb.c:757:
   for (newlength = 2; newlength + 1 < oldlength; newlength += 2)
drivers/i2c/meson_i2c.c:135:
   for (i = 0; i + 1 < i2c->count; i++)
drivers/usb/emul/usb-emul-uclass.c:21:
   for (ptr = 2, i = 0; ptr + 1 < length && *str; i++, ptr += 2) {

As

     presskey_max < MAX_DELAY_STOP_STR < INT_MAX

your suggested code will work correctly. It is just a matter of taste.

Best regards

Heinrich

>>
>> Best Regards
>> Andy Wu
>>
>>> -----Original Message-----
>>> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Heinrich
>>> Schuchardt
>>> Sent: Friday, January 15, 2021 8:19 PM
>>> To: Mo, Yuezhang <Yuezhang.Mo at sony.com>; u-boot at lists.denx.de
>>> Cc: sjg at chromium.org; hs at denx.de
>>> Subject: Re: [PATCH] autoboot: fix illegal memory access when stop key
>>> and delay key are empty
>>>
>>> On 15.01.21 04:11, Yuezhang.Mo at sony.com wrote:
>>>> If both stop key and delay key are empty, the length of these keys
>>>> is 0. The subtraction operation will cause the u_int type variable
>>>> to overflow, will cause illegal memory access in key input loop.
>>>>
>>>> This commit fixes this bug by using int type instead of u_init.
>>>> ---
>>>>   common/autoboot.c | 6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/common/autoboot.c b/common/autoboot.c index
>>>> e628baffb8..61fb09f910 100644
>>>> --- a/common/autoboot.c
>>>> +++ b/common/autoboot.c
>>>> @@ -156,9 +156,9 @@ static int passwd_abort_key(uint64_t etime)
>>>>   	};
>>>>
>>>>   	char presskey[MAX_DELAY_STOP_STR];
>>>> -	u_int presskey_len = 0;
>>>> -	u_int presskey_max = 0;
>>>> -	u_int i;
>>>> +	int presskey_len = 0;
>>>> +	int presskey_max = 0;
>>>
>>> Both indices cannot be negative. So it is understandable that u_int was chosen.
>>> You could avoid the subtraction instead of changing the type:
>>>
>>> -for (i = 0; i < presskey_max - 1; i++)
>>> +for (i = 0; i + 1 < presskey_max; i++)
>>>
>>> Acked-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>
>>>> +	int i;
>>>>
>>>>   #  ifdef CONFIG_AUTOBOOT_DELAY_STR
>>>>   	if (delaykey[0].str == NULL)
>>>>
>