* [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty @ 2021-01-15 3:11 Yuezhang.Mo at sony.com 2021-01-15 12:19 ` Heinrich Schuchardt 2021-01-28 23:58 ` Tom Rini 0 siblings, 2 replies; 6+ messages in thread From: Yuezhang.Mo at sony.com @ 2021-01-15 3:11 UTC (permalink / raw) To: u-boot 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; + int i; # ifdef CONFIG_AUTOBOOT_DELAY_STR if (delaykey[0].str == NULL) -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty 2021-01-15 3:11 [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty Yuezhang.Mo at sony.com @ 2021-01-15 12:19 ` Heinrich Schuchardt 2021-01-18 5:22 ` Andy.Wu at sony.com 2021-01-28 23:58 ` Tom Rini 1 sibling, 1 reply; 6+ messages in thread From: Heinrich Schuchardt @ 2021-01-15 12:19 UTC (permalink / raw) To: u-boot 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@gmx.de> > + int i; > > # ifdef CONFIG_AUTOBOOT_DELAY_STR > if (delaykey[0].str == NULL) > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty 2021-01-15 12:19 ` Heinrich Schuchardt @ 2021-01-18 5:22 ` Andy.Wu at sony.com 2021-01-18 5:38 ` Andy.Wu at sony.com 0 siblings, 1 reply; 6+ messages in thread From: Andy.Wu at sony.com @ 2021-01-18 5:22 UTC (permalink / raw) To: u-boot > 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 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@gmx.de> > > > + int i; > > > > # ifdef CONFIG_AUTOBOOT_DELAY_STR > > if (delaykey[0].str == NULL) > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty 2021-01-18 5:22 ` Andy.Wu at sony.com @ 2021-01-18 5:38 ` Andy.Wu at sony.com 2021-01-18 7:37 ` Heinrich Schuchardt 0 siblings, 1 reply; 6+ messages in thread From: Andy.Wu at sony.com @ 2021-01-18 5:38 UTC (permalink / raw) To: u-boot > > 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 at sony.com > Sent: Monday, January 18, 2021 1:22 PM > To: xypron.glpk at gmx.de; Mo, Yuezhang <Yuezhang.Mo@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? > > 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 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@gmx.de> > > > > > + int i; > > > > > > # ifdef CONFIG_AUTOBOOT_DELAY_STR > > > if (delaykey[0].str == NULL) > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty 2021-01-18 5:38 ` Andy.Wu at sony.com @ 2021-01-18 7:37 ` Heinrich Schuchardt 0 siblings, 0 replies; 6+ messages in thread From: Heinrich Schuchardt @ 2021-01-18 7:37 UTC (permalink / raw) To: u-boot 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@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@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@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 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@gmx.de> >>> >>>> + int i; >>>> >>>> # ifdef CONFIG_AUTOBOOT_DELAY_STR >>>> if (delaykey[0].str == NULL) >>>> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty 2021-01-15 3:11 [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty Yuezhang.Mo at sony.com 2021-01-15 12:19 ` Heinrich Schuchardt @ 2021-01-28 23:58 ` Tom Rini 1 sibling, 0 replies; 6+ messages in thread From: Tom Rini @ 2021-01-28 23:58 UTC (permalink / raw) To: u-boot On Fri, Jan 15, 2021 at 03:11:49AM +0000, 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. > Acked-by: Heinrich Schuchardt <xypron.glpk@gmx.de> Applied to u-boot/master, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210128/cee6e92b/attachment.sig> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-28 23:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-15 3:11 [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty Yuezhang.Mo at sony.com 2021-01-15 12:19 ` Heinrich Schuchardt 2021-01-18 5:22 ` Andy.Wu at sony.com 2021-01-18 5:38 ` Andy.Wu at sony.com 2021-01-18 7:37 ` Heinrich Schuchardt 2021-01-28 23:58 ` Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.