[PATCH] 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(-)
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)
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!
participants (4)
-
Andy.Wu＠sony.com -
Heinrich Schuchardt -
Tom Rini -
Yuezhang.Mo＠sony.com