KASAN: vmalloc-out-of-bounds Write in i801_isr

KASAN: vmalloc-out-of-bounds Write in i801_isr

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    040a3c33 Merge tag 'iommu-fixes-v5.5-rc5' of git://git.ker..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=16e675e1e00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=7e89bd00623fe71e
dashboard link: https://syzkaller.appspot.com/bug?extid=ed71512d469895b5b34e
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)

Unfortunately, I don't have any reproducer for this crash yet.

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+ed71512d469895b5b34e@syzkaller.appspotmail.com

BUG: KASAN: vmalloc-out-of-bounds in i801_isr_byte_done  
drivers/i2c/busses/i2c-i801.c:592 [inline]
BUG: KASAN: vmalloc-out-of-bounds in i801_isr  
drivers/i2c/busses/i2c-i801.c:663 [inline]
BUG: KASAN: vmalloc-out-of-bounds in i801_isr+0xbfd/0xcf0  
drivers/i2c/busses/i2c-i801.c:644
Write of size 1 at addr ffffc90002c07bc9 by task syz-executor.3/9465

CPU: 3 PID: 9465 Comm: syz-executor.3 Not tainted 5.5.0-rc5-syzkaller #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS  
rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
Call Trace:
  <IRQ>
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x197/0x210 lib/dump_stack.c:118
  print_address_description.constprop.0.cold+0x5/0x30b mm/kasan/report.c:374
  __kasan_report.cold+0x1b/0x41 mm/kasan/report.c:506
  kasan_report+0x12/0x20 mm/kasan/common.c:639
  __asan_report_store1_noabort+0x17/0x20 mm/kasan/generic_report.c:137
  i801_isr_byte_done drivers/i2c/busses/i2c-i801.c:592 [inline]
  i801_isr drivers/i2c/busses/i2c-i801.c:663 [inline]
  i801_isr+0xbfd/0xcf0 drivers/i2c/busses/i2c-i801.c:644
  __handle_irq_event_percpu+0x15d/0x970 kernel/irq/handle.c:149
  handle_irq_event_percpu+0x74/0x160 kernel/irq/handle.c:189
  handle_irq_event+0xa7/0x134 kernel/irq/handle.c:206
  handle_fasteoi_irq+0x281/0x670 kernel/irq/chip.c:725
  generic_handle_irq_desc include/linux/irqdesc.h:156 [inline]
  do_IRQ+0xde/0x280 arch/x86/kernel/irq.c:250
  common_interrupt+0xf/0xf arch/x86/entry/entry_64.S:607
  </IRQ>
RIP: 0010:__sanitizer_cov_trace_const_cmp4+0xd/0x20 kernel/kcov.c:274
Code: d6 0f b7 f7 bf 03 00 00 00 48 89 e5 48 8b 4d 08 e8 d8 fe ff ff 5d c3  
66 0f 1f 44 00 00 55 89 f2 89 fe bf 05 00 00 00 48 89 e5 <48> 8b 4d 08 e8  
ba fe ff ff 5d c3 0f 1f 84 00 00 00 00 00 55 48 89
RSP: 0018:ffffc90001d4f860 EFLAGS: 00000246 ORIG_RAX: ffffffffffffffdc
RAX: ffff88805df60100 RBX: 000000000000000c RCX: 000000000000000c
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000005
RBP: ffffc90001d4f860 R08: ffff88805df60100 R09: fffff520003a9f23
R10: fffff520003a9f22 R11: 0000000000000000 R12: ffff88802057db00
R13: 0000000000000030 R14: 00000000000002e6 R15: 0000000000000000
  tomoyo_domain_quota_is_ok+0x312/0x540 security/tomoyo/util.c:1071
  tomoyo_supervisor+0x2e8/0xef0 security/tomoyo/common.c:2089
  tomoyo_audit_path_log security/tomoyo/file.c:168 [inline]
  tomoyo_path_permission security/tomoyo/file.c:587 [inline]
  tomoyo_path_permission+0x263/0x360 security/tomoyo/file.c:573
  tomoyo_path_perm+0x318/0x430 security/tomoyo/file.c:838
  tomoyo_path_unlink+0x9b/0xe0 security/tomoyo/tomoyo.c:156
  security_path_unlink+0xfa/0x160 security/security.c:1044
  do_unlinkat+0x3b7/0x6d0 fs/namei.c:4064
  __do_sys_unlink fs/namei.c:4114 [inline]
  __se_sys_unlink fs/namei.c:4112 [inline]
  __x64_sys_unlink+0x42/0x50 fs/namei.c:4112
  do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x45ad27
Code: 00 66 90 b8 58 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 5d b4 fb ff c3  
66 2e 0f 1f 84 00 00 00 00 00 66 90 b8 57 00 00 00 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 3d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffebdeee5d8 EFLAGS: 00000206 ORIG_RAX: 0000000000000057
RAX: ffffffffffffffda RBX: 00007ffebdeee6a0 RCX: 000000000045ad27
RDX: 00007ffebdeee610 RSI: 00007ffebdeee610 RDI: 00007ffebdeee6a0
RBP: 0000000000000000 R08: 000000000137b983 R09: 0000000000000011
R10: ffffffffffffffff R11: 0000000000000206 R12: 00007ffebdeef730
R13: 000000000137b940 R14: 0000000000390ffe R15: 00000000ffffffff


Memory state around the buggy address:
  ffffc90002c07a80: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  ffffc90002c07b00: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
> ffffc90002c07b80: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
                                               ^
  ffffc90002c07c00: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  ffffc90002c07c80: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
==================================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

[PATCH] i2c: i801: Fix memory corruption in i801_isr_byte_done()

[Dan Carpenter]

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(-)

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

Re: [PATCH] i2c: i801: Fix memory corruption in i801_isr_byte_done()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: i801: Fix memory corruption in i801_isr_byte_done()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

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
> > 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: i801: Fix memory corruption in i801_isr_byte_done()

[Jean Delvare]

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
> > >   

Re: [PATCH] i2c: i801: Fix memory corruption in i801_isr_byte_done()

[Andy Shevchenko]

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/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: i801: Fix memory corruption in i801_isr_byte_done()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: i801: Fix memory corruption in i801_isr_byte_done()

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Fix memory corruption in i801_isr_byte_done()

[Dan Carpenter]

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

Re: [PATCH] i2c: i801: Fix memory corruption in i801_isr_byte_done()

[Jean Delvare]

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?

-- 
Jean Delvare
SUSE L3 Support