From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy.Wu at sony.com Date: Mon, 18 Jan 2021 05:22:26 +0000 Subject: [PATCH] autoboot: fix illegal memory access when stop key and delay key are empty In-Reply-To: <7b1208ef-ab69-6247-9f5d-689703199b35@gmx.de> References: <7b1208ef-ab69-6247-9f5d-689703199b35@gmx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de > 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 On Behalf Of Heinrich > Schuchardt > Sent: Friday, January 15, 2021 8:19 PM > To: Mo, Yuezhang ; 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 > > > + int i; > > > > # ifdef CONFIG_AUTOBOOT_DELAY_STR > > if (delaykey[0].str == NULL) > >