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