* [PATCH] i2c: fix stack buffer overflow vulnerability in i2c md command
@ 2022-06-10 14:50 nicolas.iooss.ledger
2022-06-21 14:04 ` Nicolas IOOSS
2022-06-28 22:53 ` Tom Rini
0 siblings, 2 replies; 5+ messages in thread
From: nicolas.iooss.ledger @ 2022-06-10 14:50 UTC (permalink / raw)
To: Simon Glass, Heiko Schocher, u-boot; +Cc: Nicolas Iooss
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: fix stack buffer overflow vulnerability in i2c md command
2022-06-10 14:50 [PATCH] i2c: fix stack buffer overflow vulnerability in i2c md command nicolas.iooss.ledger
@ 2022-06-21 14:04 ` Nicolas IOOSS
2022-06-27 4:33 ` Heiko Schocher
2022-06-28 22:53 ` Tom Rini
1 sibling, 1 reply; 5+ messages in thread
From: Nicolas IOOSS @ 2022-06-21 14:04 UTC (permalink / raw)
To: Simon Glass, Heiko Schocher; +Cc: u-boot, Nicolas Iooss
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: fix stack buffer overflow vulnerability in i2c md command
2022-06-21 14:04 ` Nicolas IOOSS
@ 2022-06-27 4:33 ` Heiko Schocher
2022-06-27 20:46 ` Tom Rini
0 siblings, 1 reply; 5+ messages in thread
From: Heiko Schocher @ 2022-06-27 4:33 UTC (permalink / raw)
To: Nicolas IOOSS, Simon Glass, Tom Rini; +Cc: u-boot, Nicolas Iooss
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
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: fix stack buffer overflow vulnerability in i2c md command
2022-06-27 4:33 ` Heiko Schocher
@ 2022-06-27 20:46 ` Tom Rini
0 siblings, 0 replies; 5+ messages in thread
From: Tom Rini @ 2022-06-27 20:46 UTC (permalink / raw)
To: Heiko Schocher; +Cc: Nicolas IOOSS, Simon Glass, u-boot, Nicolas Iooss
[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]
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.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: fix stack buffer overflow vulnerability in i2c md command
2022-06-10 14:50 [PATCH] i2c: fix stack buffer overflow vulnerability in i2c md command nicolas.iooss.ledger
2022-06-21 14:04 ` Nicolas IOOSS
@ 2022-06-28 22:53 ` Tom Rini
1 sibling, 0 replies; 5+ messages in thread
From: Tom Rini @ 2022-06-28 22:53 UTC (permalink / raw)
To: nicolas.iooss.ledger; +Cc: Simon Glass, Heiko Schocher, u-boot, Nicolas Iooss
[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]
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!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-28 22:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-10 14:50 [PATCH] i2c: fix stack buffer overflow vulnerability in i2c md command nicolas.iooss.ledger
2022-06-21 14:04 ` Nicolas IOOSS
2022-06-27 4:33 ` Heiko Schocher
2022-06-27 20:46 ` Tom Rini
2022-06-28 22:53 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.