[PATCH] i2c: fix stack buffer overflow vulnerability in i2c md command
From: Nicolas Iooss nicolas.iooss+uboot@ledger.fr
When running "i2c md 0 0 80000100", the function do_i2c_md parses the length into an unsigned int variable named length. The value is then moved to a signed variable:
int nbytes = length; #define DISP_LINE_LEN 16 int linebytes = (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes; ret = dm_i2c_read(dev, addr, linebuf, linebytes);
On systems where integers are 32 bits wide, 0x80000100 is a negative value to "nbytes > DISP_LINE_LEN" is false and linebytes gets assigned 0x80000100 instead of 16.
The consequence is that the function which reads from the i2c device (dm_i2c_read or i2c_read) is called with a 16-byte stack buffer to fill but with a size parameter which is too large. In some cases, this could trigger a crash. But with some i2c drivers, such as drivers/i2c/nx_i2c.c (used with "nexell,s5pxx18-i2c" bus), the size is actually truncated to a 16-bit integer. This is because function i2c_transfer expects an unsigned short length. In such a case, an attacker who can control the response of an i2c device can overwrite the return address of a function and execute arbitrary code through Return-Oriented Programming.
Fix this issue by using unsigned integers types in do_i2c_md. While at it, make also alen unsigned, as signed sizes can cause vulnerabilities when people forgot to check that they can be negative.
Signed-off-by: Nicolas Iooss nicolas.iooss+uboot@ledger.fr --- cmd/i2c.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/cmd/i2c.c b/cmd/i2c.c index 9050b2b8d27a..bd04b14024be 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, int default_len) +static uint get_alen(char *arg, uint default_len) { - int j; - int alen; + uint j; + uint 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; - int alen; + uint 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; - int alen; + uint 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; - int alen; - int j, nbytes, linebytes; + uint alen; + uint 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; - int alen; + uint alen; uchar byte; - int count; + uint 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; - int alen; - int count; + uint alen; + uint 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; - int alen; + uint alen; uint addr; uint length; u_char bytes[16];
Hello,
I sent some days ago the vulnerability fix below. I have not received any reply yet. Could a maintainer take a look at it, please?
Best regards, Nicolas
------- Original Message ------- Le vendredi 10 juin 2022 à 4:50 PM, nicolas.iooss.ledger@proton.me a écrit :
From: Nicolas Iooss nicolas.iooss+uboot@ledger.fr
When running "i2c md 0 0 80000100", the function do_i2c_md parses the length into an unsigned int variable named length. The value is then moved to a signed variable:
int nbytes = length; #define DISP_LINE_LEN 16 int linebytes = (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes;
ret = dm_i2c_read(dev, addr, linebuf, linebytes);
On systems where integers are 32 bits wide, 0x80000100 is a negative value to "nbytes > DISP_LINE_LEN" is false and linebytes gets assigned
0x80000100 instead of 16.
The consequence is that the function which reads from the i2c device (dm_i2c_read or i2c_read) is called with a 16-byte stack buffer to fill but with a size parameter which is too large. In some cases, this could trigger a crash. But with some i2c drivers, such as drivers/i2c/nx_i2c.c (used with "nexell,s5pxx18-i2c" bus), the size is actually truncated to a 16-bit integer. This is because function i2c_transfer expects an unsigned short length. In such a case, an attacker who can control the response of an i2c device can overwrite the return address of a function and execute arbitrary code through Return-Oriented Programming.
Fix this issue by using unsigned integers types in do_i2c_md. While at it, make also alen unsigned, as signed sizes can cause vulnerabilities when people forgot to check that they can be negative.
Signed-off-by: Nicolas Iooss nicolas.iooss+uboot@ledger.fr
cmd/i2c.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/cmd/i2c.c b/cmd/i2c.c index 9050b2b8d27a..bd04b14024be 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, int default_len) +static uint get_alen(char *arg, uint default_len) {
- int j;
- int alen;
- uint j;
- uint 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;
- int alen;
- uint 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;
- int alen;
- uint 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;
- int alen;
- int j, nbytes, linebytes;
- uint alen;
- uint 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;
- int alen;
- uint alen;
uchar byte;
- int count;
- uint 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;
- int alen;
- int count;
- uint alen;
- uint 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;
- int alen;
- uint alen;
uint addr; uint length; u_char bytes[16]; -- 2.32.0
Hello Nicolas,
On 21.06.22 16:04, Nicolas IOOSS wrote:
Hello,
I sent some days ago the vulnerability fix below. I have not received any reply yet. Could a maintainer take a look at it, please?
Sorry for that, but I was on the road (embedded world in nuremberg).
Best regards, Nicolas
------- Original Message ------- Le vendredi 10 juin 2022 à 4:50 PM, nicolas.iooss.ledger@proton.me a écrit :
From: Nicolas Iooss nicolas.iooss+uboot@ledger.fr
When running "i2c md 0 0 80000100", the function do_i2c_md parses the length into an unsigned int variable named length. The value is then moved to a signed variable:
int nbytes = length; #define DISP_LINE_LEN 16 int linebytes = (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes;
ret = dm_i2c_read(dev, addr, linebuf, linebytes);
On systems where integers are 32 bits wide, 0x80000100 is a negative value to "nbytes > DISP_LINE_LEN" is false and linebytes gets assigned
0x80000100 instead of 16.
The consequence is that the function which reads from the i2c device (dm_i2c_read or i2c_read) is called with a 16-byte stack buffer to fill but with a size parameter which is too large. In some cases, this could trigger a crash. But with some i2c drivers, such as drivers/i2c/nx_i2c.c (used with "nexell,s5pxx18-i2c" bus), the size is actually truncated to a 16-bit integer. This is because function i2c_transfer expects an unsigned short length. In such a case, an attacker who can control the response of an i2c device can overwrite the return address of a function and execute arbitrary code through Return-Oriented Programming.
Fix this issue by using unsigned integers types in do_i2c_md. While at it, make also alen unsigned, as signed sizes can cause vulnerabilities when people forgot to check that they can be negative.
Signed-off-by: Nicolas Iooss nicolas.iooss+uboot@ledger.fr
cmd/i2c.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Heiko Schocher hs@denx.de
@Tom: Should we add this to 2022.07? If so, feel free to pick it up, thanks!
bye, Heiko
On Mon, Jun 27, 2022 at 06:33:01AM +0200, Heiko Schocher wrote:
Hello Nicolas,
On 21.06.22 16:04, Nicolas IOOSS wrote:
Hello,
I sent some days ago the vulnerability fix below. I have not received any reply yet. Could a maintainer take a look at it, please?
Sorry for that, but I was on the road (embedded world in nuremberg).
Best regards, Nicolas
------- Original Message ------- Le vendredi 10 juin 2022 à 4:50 PM, nicolas.iooss.ledger@proton.me a écrit :
From: Nicolas Iooss nicolas.iooss+uboot@ledger.fr
When running "i2c md 0 0 80000100", the function do_i2c_md parses the length into an unsigned int variable named length. The value is then moved to a signed variable:
int nbytes = length; #define DISP_LINE_LEN 16 int linebytes = (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes;
ret = dm_i2c_read(dev, addr, linebuf, linebytes);
On systems where integers are 32 bits wide, 0x80000100 is a negative value to "nbytes > DISP_LINE_LEN" is false and linebytes gets assigned
0x80000100 instead of 16.
The consequence is that the function which reads from the i2c device (dm_i2c_read or i2c_read) is called with a 16-byte stack buffer to fill but with a size parameter which is too large. In some cases, this could trigger a crash. But with some i2c drivers, such as drivers/i2c/nx_i2c.c (used with "nexell,s5pxx18-i2c" bus), the size is actually truncated to a 16-bit integer. This is because function i2c_transfer expects an unsigned short length. In such a case, an attacker who can control the response of an i2c device can overwrite the return address of a function and execute arbitrary code through Return-Oriented Programming.
Fix this issue by using unsigned integers types in do_i2c_md. While at it, make also alen unsigned, as signed sizes can cause vulnerabilities when people forgot to check that they can be negative.
Signed-off-by: Nicolas Iooss nicolas.iooss+uboot@ledger.fr
cmd/i2c.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Heiko Schocher hs@denx.de
@Tom: Should we add this to 2022.07? If so, feel free to pick it up, thanks!
I'll pick this up directly along with the squashfs fix in the next day or two, thanks.
On Fri, Jun 10, 2022 at 02:50:25PM +0000, nicolas.iooss.ledger@proton.me wrote:
From: Nicolas Iooss nicolas.iooss+uboot@ledger.fr
When running "i2c md 0 0 80000100", the function do_i2c_md parses the length into an unsigned int variable named length. The value is then moved to a signed variable:
int nbytes = length; #define DISP_LINE_LEN 16 int linebytes = (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes; ret = dm_i2c_read(dev, addr, linebuf, linebytes);On systems where integers are 32 bits wide, 0x80000100 is a negative value to "nbytes > DISP_LINE_LEN" is false and linebytes gets assigned 0x80000100 instead of 16.
The consequence is that the function which reads from the i2c device (dm_i2c_read or i2c_read) is called with a 16-byte stack buffer to fill but with a size parameter which is too large. In some cases, this could trigger a crash. But with some i2c drivers, such as drivers/i2c/nx_i2c.c (used with "nexell,s5pxx18-i2c" bus), the size is actually truncated to a 16-bit integer. This is because function i2c_transfer expects an unsigned short length. In such a case, an attacker who can control the response of an i2c device can overwrite the return address of a function and execute arbitrary code through Return-Oriented Programming.
Fix this issue by using unsigned integers types in do_i2c_md. While at it, make also alen unsigned, as signed sizes can cause vulnerabilities when people forgot to check that they can be negative.
Signed-off-by: Nicolas Iooss nicolas.iooss+uboot@ledger.fr Reviewed-by: Heiko Schocher hs@denx.de
Applied to u-boot/master, thanks!
participants (4)
-
Heiko Schocher -
Nicolas IOOSS -
nicolas.iooss.ledger＠proton.me
-
Tom Rini