linux-kernel - Re: [PATCH] i2c: i801: Fix memory corruption in i801_isr_byte

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200322231106.3d431ced@endymion>
Date:   Sun, 22 Mar 2020 23:11:06 +0100
From:   Jean Delvare <jdelvare@...e.de>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Daniel Kurtz <djkurtz@...omium.org>, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        syzbot <syzbot+ed71512d469895b5b34e@...kaller.appspotmail.com>
Subject: Re: [PATCH] i2c: i801: Fix memory corruption in
 i801_isr_byte_done()

Hi Dan,

On Tue, 14 Jan 2020 10:34:06 +0300, Dan Carpenter wrote:
> Assigning "priv->data[-1] = priv->len;" obviously doesn't make sense.
> What it does is it ends up corrupting the last byte of priv->len so
> priv->len becomes a very high number.

I don't follow you, sorry. "priv->data[-1] = priv->len" is writing to
priv->data, not priv->len, so I can't see how this could corrupt
priv->len;

Yes, I see that len is right before data in struct i801_priv, however
priv->data is a pointer, not an array inside the structure, it points
outside the structure so whatever is done through that pointer can't
affect the structure's content.

As for priv->data[-1], in priv->data is defined as:

		priv->data = &data->block[1];

which means the pointer is 1 byte inside the actual block array,
therefore priv->data[-1] albeit convoluted looks legal to me.

> Reported-by: syzbot+ed71512d469895b5b34e@...kaller.appspotmail.com
> Fixes: d3ff6ce40031 ("i2c-i801: Enable IRQ for byte_by_byte transactions")
> Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> ---
> Untested.
> 
>  drivers/i2c/busses/i2c-i801.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f5e69fe56532..420d8025901e 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -584,7 +584,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>  					"SMBus block read size is %d\n",
>  					priv->len);
>  			}
> -			priv->data[-1] = priv->len;
>  		}
>  
>  		/* Read next byte */

Definitely not correct. The first byte of the block data array MUST be
the size of the block read. Even if the code above does not do the
right thing, removing the line will not help.

Is it possible that kasan got this wrong due to the convoluted logic?
It's late and I'll check again tomorrow morning but the code looks OK
to me.

-- 
Jean Delvare
SUSE L3 Support