| Message ID | 20200114073406.qaq3hbrhtx76fkes@kili.mountain |
|---|---|
| State | Rejected |
| Headers | show |
| Series | i2c: i801: Fix memory corruption in i801_isr_byte_done() | expand |
On Tue, Jan 14, 2020 at 10:34:06AM +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. > > Reported-by: syzbot+ed71512d469895b5b34e@syzkaller.appspotmail.com > Fixes: d3ff6ce40031 ("i2c-i801: Enable IRQ for byte_by_byte transactions") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- Daniel, Jean: what do you think? Also, adding Jarkko to CC who works a lot with this driver... > 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 */ > -- > 2.11.0 >
On Sat, Feb 22, 2020 at 01:45:23PM +0100, Wolfram Sang wrote: > On Tue, Jan 14, 2020 at 10:34:06AM +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. > > > > Reported-by: syzbot+ed71512d469895b5b34e@syzkaller.appspotmail.com > > Fixes: d3ff6ce40031 ("i2c-i801: Enable IRQ for byte_by_byte transactions") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > Daniel, Jean: what do you think? > Also, adding Jarkko to CC who works a lot with this driver... Ping. Adding more people... > > > 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 */ > > -- > > 2.11.0 > >
Hi Wolfram, Can you please bounce the previous messages in this thread to me? I was apparently not Cc'd so I'm missing the context. Thanks, Jean On Fri, 20 Mar 2020 15:57:48 +0100, Wolfram Sang wrote: > On Sat, Feb 22, 2020 at 01:45:23PM +0100, Wolfram Sang wrote: > > On Tue, Jan 14, 2020 at 10:34:06AM +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. > > > > > > Reported-by: syzbot+ed71512d469895b5b34e@syzkaller.appspotmail.com > > > Fixes: d3ff6ce40031 ("i2c-i801: Enable IRQ for byte_by_byte transactions") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > > Daniel, Jean: what do you think? > > Also, adding Jarkko to CC who works a lot with this driver... > > Ping. Adding more people... > > > > > > 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 */ > > > -- > > > 2.11.0 > > >
On Sun, Mar 22, 2020 at 8:11 PM Jean Delvare <jdelvare@suse.de> wrote: > > Hi Wolfram, > > Can you please bounce the previous messages in this thread to me? I was > apparently not Cc'd so I'm missing the context. You can always take it from patchwork http://patchwork.ozlabs.org/patch/1222542/
> Can you please bounce the previous messages in this thread to me? I was > apparently not Cc'd so I'm missing the context. Sure, done. Your email address in the thread was sadly the outdated one.
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@syzkaller.appspotmail.com > Fixes: d3ff6ce40031 ("i2c-i801: Enable IRQ for byte_by_byte transactions") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.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.
On Sun, Mar 22, 2020 at 11:11:06PM +0100, Jean Delvare wrote: > 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. > Yeah. I misread the code. > 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. KASan doesn't work like that. It works at runtime and doesn't care about the logic. https://syzkaller.appspot.com/bug?id=426fc8b1c1b63fb0af524d839dfcf452f2d858e2 At the bottom of the report it shows that we're in a field of f9 poisoned data so it's not priv->len which is wrong. (My patch was way off). mm/kasan/kasan.h:#define KASAN_VMALLOC_INVALID 0xF9 /* unallocated space in vmapped page */ The logic looks okay to me too. So possibly this was a race condition or even memory corruption in an unrelated part of the kernel. regards, dan carpenter
On Mon, 23 Mar 2020 12:37:33 +0300, Dan Carpenter wrote: > On Sun, Mar 22, 2020 at 11:11:06PM +0100, Jean Delvare wrote: > > 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. > > > > Yeah. I misread the code. > > > 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. > > KASan doesn't work like that. It works at runtime and doesn't care > about the logic. > > https://syzkaller.appspot.com/bug?id=426fc8b1c1b63fb0af524d839dfcf452f2d858e2 > > At the bottom of the report it shows that we're in a field of f9 > poisoned data so it's not priv->len which is wrong. (My patch was way > off). > > mm/kasan/kasan.h:#define KASAN_VMALLOC_INVALID 0xF9 /* unallocated space in vmapped page */ > > The logic looks okay to me too. So possibly this was a race condition > or even memory corruption in an unrelated part of the kernel. I checked out the exact kernel version this report was generated for, and the faulty line is: 592: priv->data[priv->count++] = inb(SMBBLKDAT(priv)); This would suggest the problem is with priv->count growing beyond the end of the array, however the fact that we land in a memory spot full of 0xF9 kind of excludes this possibility (the data before the spot would contain different data if it was the case). The other option is that priv->count wasn't initialized at the time it is used. However I can't see how this could happen, given that the priv structure is kzalloc'd. So, to be honest I can't really see how priv->count can get wrong. So I would be tempted to lend towards the theory that the i2c-i801 driver was a collateral victim of a memory corruption happening somewhere else in the kernel. Wouldn't Kasan catch this too? Is it possible to access the other Kasan reports from the same test run?
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 */
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. Reported-by: syzbot+ed71512d469895b5b34e@syzkaller.appspotmail.com Fixes: d3ff6ce40031 ("i2c-i801: Enable IRQ for byte_by_byte transactions") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- Untested. drivers/i2c/busses/i2c-i801.c | 1 - 1 file changed, 1 deletion(-)