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

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

On 15.01.21 04:11, Yuezhang.Mo@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@gmx.de>

> +	int i;
>
>  #  ifdef CONFIG_AUTOBOOT_DELAY_STR
>  	if (delaykey[0].str == NULL)
>

> 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?

Best Regards
Andy Wu

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Heinrich
> Schuchardt
> Sent: Friday, January 15, 2021 8:19 PM
> To: Mo, Yuezhang <Yuezhang.Mo@sony.com>; u-boot@lists.denx.de
> Cc: sjg@chromium.org; hs@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@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@gmx.de>
> 
> > +	int i;
> >
> >  #  ifdef CONFIG_AUTOBOOT_DELAY_STR
> >  	if (delaykey[0].str == NULL)
> >

> > 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@lists.denx.de> On Behalf Of
> Andy.Wu@sony.com
> Sent: Monday, January 18, 2021 1:22 PM
> To: xypron.glpk@gmx.de; Mo, Yuezhang <Yuezhang.Mo@sony.com>;
> u-boot@lists.denx.de
> Cc: sjg@chromium.org; hs@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?
> 
> Best Regards
> Andy Wu
> 
> > -----Original Message-----
> > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Heinrich
> > Schuchardt
> > Sent: Friday, January 15, 2021 8:19 PM
> > To: Mo, Yuezhang <Yuezhang.Mo@sony.com>; u-boot@lists.denx.de
> > Cc: sjg@chromium.org; hs@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@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@gmx.de>
> >
> > > +	int i;
> > >
> > >  #  ifdef CONFIG_AUTOBOOT_DELAY_STR
> > >  	if (delaykey[0].str == NULL)
> > >

On 1/18/21 6:38 AM, Andy.Wu@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@lists.denx.de> On Behalf Of
>> Andy.Wu@sony.com
>> Sent: Monday, January 18, 2021 1:22 PM
>> To: xypron.glpk@gmx.de; Mo, Yuezhang <Yuezhang.Mo@sony.com>;
>> u-boot@lists.denx.de
>> Cc: sjg@chromium.org; hs@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@lists.denx.de> On Behalf Of Heinrich
>>> Schuchardt
>>> Sent: Friday, January 15, 2021 8:19 PM
>>> To: Mo, Yuezhang <Yuezhang.Mo@sony.com>; u-boot@lists.denx.de
>>> Cc: sjg@chromium.org; hs@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@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@gmx.de>
>>>
>>>> +	int i;
>>>>
>>>>   #  ifdef CONFIG_AUTOBOOT_DELAY_STR
>>>>   	if (delaykey[0].str == NULL)
>>>>
>

On Fri, Jan 15, 2021 at 03:11:49AM +0000, Yuezhang.Mo@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.
> Acked-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Applied to u-boot/master, thanks!