[PATCH 1/2] Revert "i2c: fix stack buffer overflow vulnerability in i2c md command"
This reverts commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409.
The commit is largely wrong and breaks most of i2c command functionality. The problem described in the aforementioned commit commit message is valid, however the commit itself does many more changes unrelated to fixing that one problem it describes. Those extra changes, namely the handling of i2c device address length as unsigned instead of signed integer, breaks the expectation that address length may be negative value. The negative value is used by DM to indicate that address length of device does not change.
The actual bug documented in commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 can be fixed by extra sanitization in separate patch.
Signed-off-by: Marek Vasut marex@denx.de Cc: Heiko Schocher hs@denx.de Cc: Nicolas Iooss nicolas.iooss+uboot@ledger.fr Cc: Simon Glass sjg@chromium.org Cc: Tim Harvey tharvey@gateworks.com --- cmd/i2c.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/cmd/i2c.c b/cmd/i2c.c index bd04b14024b..9050b2b8d27 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -200,10 +200,10 @@ void i2c_init_board(void) * * Returns the address length. */ -static uint get_alen(char *arg, uint default_len) +static uint get_alen(char *arg, int default_len) { - uint j; - uint alen; + int j; + int alen;
alen = default_len; for (j = 0; j < 8; j++) { @@ -247,7 +247,7 @@ static int do_i2c_read(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; uint devaddr, length; - uint alen; + int alen; u_char *memaddr; int ret; #if CONFIG_IS_ENABLED(DM_I2C) @@ -301,7 +301,7 @@ static int do_i2c_write(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; uint devaddr, length; - uint alen; + int alen; u_char *memaddr; int ret; #if CONFIG_IS_ENABLED(DM_I2C) @@ -469,8 +469,8 @@ static int do_i2c_md(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; uint addr, length; - uint alen; - uint j, nbytes, linebytes; + int alen; + int j, nbytes, linebytes; int ret; #if CONFIG_IS_ENABLED(DM_I2C) struct udevice *dev; @@ -589,9 +589,9 @@ static int do_i2c_mw(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; ulong addr; - uint alen; + int alen; uchar byte; - uint count; + int count; int ret; #if CONFIG_IS_ENABLED(DM_I2C) struct udevice *dev; @@ -676,8 +676,8 @@ static int do_i2c_crc(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; ulong addr; - uint alen; - uint count; + int alen; + int count; uchar byte; ulong crc; ulong err; @@ -985,7 +985,7 @@ static int do_i2c_loop(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { uint chip; - uint alen; + int alen; uint addr; uint length; u_char bytes[16];
This reinstates fix from commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 without the changes unrelated to the actual fix. Avoid the underflow by setting only nbytes and linebytes as unsigned integers.
Signed-off-by: Marek Vasut marex@denx.de Cc: Heiko Schocher hs@denx.de Cc: Nicolas Iooss nicolas.iooss+uboot@ledger.fr Cc: Simon Glass sjg@chromium.org Cc: Tim Harvey tharvey@gateworks.com --- cmd/i2c.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/cmd/i2c.c b/cmd/i2c.c index 9050b2b8d27..e196a73efa6 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -470,7 +470,8 @@ static int do_i2c_md(struct cmd_tbl *cmdtp, int flag, int argc, uint chip; uint addr, length; int alen; - int j, nbytes, linebytes; + int j; + uint nbytes, linebytes; int ret; #if CONFIG_IS_ENABLED(DM_I2C) struct udevice *dev;
On Fri, Aug 26, 2022 at 2:16 PM Marek Vasut marex@denx.de wrote:
This reinstates fix from commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 without the changes unrelated to the actual fix. Avoid the underflow by setting only nbytes and linebytes as unsigned integers.
Signed-off-by: Marek Vasut marex@denx.de Cc: Heiko Schocher hs@denx.de Cc: Nicolas Iooss nicolas.iooss+uboot@ledger.fr Cc: Simon Glass sjg@chromium.org Cc: Tim Harvey tharvey@gateworks.com
cmd/i2c.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/cmd/i2c.c b/cmd/i2c.c index 9050b2b8d27..e196a73efa6 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -470,7 +470,8 @@ static int do_i2c_md(struct cmd_tbl *cmdtp, int flag, int argc, uint chip; uint addr, length; int alen;
int j, nbytes, linebytes;
int j;uint nbytes, linebytes; int ret;#if CONFIG_IS_ENABLED(DM_I2C) struct udevice *dev; -- 2.35.1
Marek,
Did 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 get reverted then?
Best Regards,
Tim
On Fri, Aug 26, 2022 at 3:21 PM Tim Harvey tharvey@gateworks.com wrote:
On Fri, Aug 26, 2022 at 2:16 PM Marek Vasut marex@denx.de wrote:
This reinstates fix from commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 without the changes unrelated to the actual fix. Avoid the underflow by setting only nbytes and linebytes as unsigned integers.
Signed-off-by: Marek Vasut marex@denx.de Cc: Heiko Schocher hs@denx.de Cc: Nicolas Iooss nicolas.iooss+uboot@ledger.fr Cc: Simon Glass sjg@chromium.org Cc: Tim Harvey tharvey@gateworks.com
cmd/i2c.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/cmd/i2c.c b/cmd/i2c.c index 9050b2b8d27..e196a73efa6 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -470,7 +470,8 @@ static int do_i2c_md(struct cmd_tbl *cmdtp, int flag, int argc, uint chip; uint addr, length; int alen;
int j, nbytes, linebytes;
int j;uint nbytes, linebytes; int ret;#if CONFIG_IS_ENABLED(DM_I2C) struct udevice *dev; -- 2.35.1
Marek,
Did 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 get reverted then?
Oh gosh... my bad for not seeing your first patch that reverts it!
Acked-by: Tim Harvey tharvey@gateworks.com
Best Regards,
Tim
On Fri, Aug 26, 2022 at 11:15:56PM +0200, Marek Vasut wrote:
This reinstates fix from commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 without the changes unrelated to the actual fix. Avoid the underflow by setting only nbytes and linebytes as unsigned integers.
Signed-off-by: Marek Vasut marex@denx.de Cc: Heiko Schocher hs@denx.de Cc: Nicolas Iooss nicolas.iooss+uboot@ledger.fr Cc: Simon Glass sjg@chromium.org Cc: Tim Harvey tharvey@gateworks.com Acked-by: Tim Harvey tharvey@gateworks.com
Applied to u-boot/master, thanks!
On Fri, 26 Aug 2022 at 15:16, Marek Vasut marex@denx.de wrote:
This reverts commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409.
The commit is largely wrong and breaks most of i2c command functionality. The problem described in the aforementioned commit commit message is valid, however the commit itself does many more changes unrelated to fixing that one problem it describes. Those extra changes, namely the handling of i2c device address length as unsigned instead of signed integer, breaks the expectation that address length may be negative value. The negative value is used by DM to indicate that address length of device does not change.
The actual bug documented in commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 can be fixed by extra sanitization in separate patch.
Signed-off-by: Marek Vasut marex@denx.de Cc: Heiko Schocher hs@denx.de Cc: Nicolas Iooss nicolas.iooss+uboot@ledger.fr Cc: Simon Glass sjg@chromium.org Cc: Tim Harvey tharvey@gateworks.com
cmd/i2c.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Aug 26, 2022 at 11:15:55PM +0200, Marek Vasut wrote:
This reverts commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409.
The commit is largely wrong and breaks most of i2c command functionality. The problem described in the aforementioned commit commit message is valid, however the commit itself does many more changes unrelated to fixing that one problem it describes. Those extra changes, namely the handling of i2c device address length as unsigned instead of signed integer, breaks the expectation that address length may be negative value. The negative value is used by DM to indicate that address length of device does not change.
The actual bug documented in commit 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 can be fixed by extra sanitization in separate patch.
Signed-off-by: Marek Vasut marex@denx.de Cc: Heiko Schocher hs@denx.de Cc: Nicolas Iooss nicolas.iooss+uboot@ledger.fr Cc: Simon Glass sjg@chromium.org Cc: Tim Harvey tharvey@gateworks.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (4)
-
Marek Vasut -
Simon Glass -
Tim Harvey -
Tom Rini