Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* KMSAN: uninit-value in alauda_check_media
@ 2019-10-07 19:39 syzbot
  2019-10-11 11:23 ` Jaskaran Singh
  0 siblings, 1 reply; 11+ messages in thread
From: syzbot @ 2019-10-07 19:39 UTC (permalink / raw)
  To: glider, gregkh, linux-kernel, linux-usb, stern, syzkaller-bugs,
	usb-storage

Hello,

syzbot found the following crash on:

HEAD commit:    1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with kmsan_i..
git tree:       https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
kernel config:  https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
dashboard link: https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler:       clang version 9.0.0 (/home/glider/llvm/clang  
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=110631b7600000

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

=====================================================
BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0  
drivers/usb/storage/alauda.c:1137
CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
  usb_stor_invoke_transport+0xf5/0x27e0 drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355

Local variable description: ----status@alauda_check_media
Variable was created at:
  alauda_check_media+0x8e/0x3310 drivers/usb/storage/alauda.c:454
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
=====================================================
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 12279 Comm: usb-storage Tainted: G    B             5.3.0-rc7+  
#0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  panic+0x3c9/0xc1e kernel/panic.c:219
  kmsan_report+0x2a2/0x2b0 mm/kmsan/kmsan_report.c:131
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
  usb_stor_invoke_transport+0xf5/0x27e0 drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
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.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
  2019-10-07 19:39 KMSAN: uninit-value in alauda_check_media syzbot
@ 2019-10-11 11:23 ` Jaskaran Singh
  2019-10-11 11:51   ` Alexander Potapenko
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Jaskaran Singh @ 2019-10-11 11:23 UTC (permalink / raw)
  To: syzbot, glider, gregkh, linux-kernel, linux-usb, stern,
	syzkaller-bugs, usb-storage

On Mon, 2019-10-07 at 12:39 -0700, syzbot wrote:
> Hello,
> 
> syzbot found the following crash on:
> 
> HEAD commit:    1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with
> kmsan_i..
> git tree:       https://github.com/google/kmsan.git master
> console output: 
> https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
> kernel config:  
> https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
> dashboard link: 
> https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
> compiler:       clang version 9.0.0 (/home/glider/llvm/clang  
> 80fee25776c2fb61e74c1ecb1a523375c2500b69)
> syz repro:      
> https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
> C reproducer:   
> https://syzkaller.appspot.com/x/repro.c?x=110631b7600000
> 
> IMPORTANT: if you fix the bug, please add the following tag to the
> commit:
> Reported-by: syzbot+e7d46eb426883fb97efd@syzkaller.appspotmail.com
> 
> =====================================================
> BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0  
> drivers/usb/storage/alauda.c:1137
> CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
> Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS  
> Google 01/01/2011
> Call Trace:
>   __dump_stack lib/dump_stack.c:77 [inline]
>   dump_stack+0x191/0x1f0 lib/dump_stack.c:113
>   kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
>   __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
>   alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
>   alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
>   usb_stor_invoke_transport+0xf5/0x27e0
> drivers/usb/storage/transport.c:606
>   usb_stor_transparent_scsi_command+0x5d/0x70  
> drivers/usb/storage/protocol.c:108
>   usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
>   kthread+0x4b5/0x4f0 kernel/kthread.c:256
>   ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
> 
> Local variable description: ----status@alauda_check_media
> Variable was created at:
>   alauda_check_media+0x8e/0x3310 drivers/usb/storage/alauda.c:454
>   alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
> =====================================================
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 0 PID: 12279 Comm: usb-storage Tainted:
> G    B             5.3.0-rc7+  
> #0
> Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS  
> Google 01/01/2011
> Call Trace:
>   __dump_stack lib/dump_stack.c:77 [inline]
>   dump_stack+0x191/0x1f0 lib/dump_stack.c:113
>   panic+0x3c9/0xc1e kernel/panic.c:219
>   kmsan_report+0x2a2/0x2b0 mm/kmsan/kmsan_report.c:131
>   __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
>   alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
>   alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
>   usb_stor_invoke_transport+0xf5/0x27e0
> drivers/usb/storage/transport.c:606
>   usb_stor_transparent_scsi_command+0x5d/0x70  
> drivers/usb/storage/protocol.c:108
>   usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
>   kthread+0x4b5/0x4f0 kernel/kthread.c:256
>   ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
> Kernel Offset: disabled
> Rebooting in 86400 seconds..
> 
> 
> ---
> 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.
> syzbot can test patches for this bug, for details see:
> https://goo.gl/tpsmEJ#testing-patches

#syz test: https://github.com/google/kmsan.git 1e76a3e5

diff --git a/drivers/usb/storage/alauda.c
b/drivers/usb/storage/alauda.c
index ddab2cd3d2e7..bb309b9ad65b 100644
--- a/drivers/usb/storage/alauda.c
+++ b/drivers/usb/storage/alauda.c
@@ -452,7 +452,7 @@ static int alauda_init_media(struct us_data *us)
 static int alauda_check_media(struct us_data *us)
 {
 	struct alauda_info *info = (struct alauda_info *) us->extra;
-	unsigned char status[2];
+	unsigned char *status = us->iobuf;
 	int rc;
 
 	rc = alauda_get_media_status(us, status);


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
  2019-10-11 11:23 ` Jaskaran Singh
@ 2019-10-11 11:51   ` Alexander Potapenko
  2019-10-11 15:42     ` syzbot
  2019-10-11 14:08   ` Alan Stern
  2019-10-11 15:24   ` syzbot
  2 siblings, 1 reply; 11+ messages in thread
From: Alexander Potapenko @ 2019-10-11 11:51 UTC (permalink / raw)
  To: Jaskaran Singh
  Cc: syzbot, Greg Kroah-Hartman, LKML, USB list, Alan Stern,
	syzkaller-bugs, usb-storage

On Fri, Oct 11, 2019 at 1:23 PM Jaskaran Singh
<jaskaransingh7654321@gmail.com> wrote:
>
> On Mon, 2019-10-07 at 12:39 -0700, syzbot wrote:
> > Hello,
> >
> > syzbot found the following crash on:
> >
> > HEAD commit:    1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with
> > kmsan_i..
> > git tree:       https://github.com/google/kmsan.git master
> > console output:
> > https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
> > kernel config:
> > https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
> > dashboard link:
> > https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
> > compiler:       clang version 9.0.0 (/home/glider/llvm/clang
> > 80fee25776c2fb61e74c1ecb1a523375c2500b69)
> > syz repro:
> > https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
> > C reproducer:
> > https://syzkaller.appspot.com/x/repro.c?x=110631b7600000
> >
> > IMPORTANT: if you fix the bug, please add the following tag to the
> > commit:
> > Reported-by: syzbot+e7d46eb426883fb97efd@syzkaller.appspotmail.com
> >
> > =====================================================
> > BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0
> > drivers/usb/storage/alauda.c:1137
> > CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine,
> > BIOS
> > Google 01/01/2011
> > Call Trace:
> >   __dump_stack lib/dump_stack.c:77 [inline]
> >   dump_stack+0x191/0x1f0 lib/dump_stack.c:113
> >   kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
> >   __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
> >   alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
> >   alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
> >   usb_stor_invoke_transport+0xf5/0x27e0
> > drivers/usb/storage/transport.c:606
> >   usb_stor_transparent_scsi_command+0x5d/0x70
> > drivers/usb/storage/protocol.c:108
> >   usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
> >   kthread+0x4b5/0x4f0 kernel/kthread.c:256
> >   ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
> >
> > Local variable description: ----status@alauda_check_media
> > Variable was created at:
> >   alauda_check_media+0x8e/0x3310 drivers/usb/storage/alauda.c:454
> >   alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
> > =====================================================
> > Kernel panic - not syncing: panic_on_warn set ...
> > CPU: 0 PID: 12279 Comm: usb-storage Tainted:
> > G    B             5.3.0-rc7+
> > #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine,
> > BIOS
> > Google 01/01/2011
> > Call Trace:
> >   __dump_stack lib/dump_stack.c:77 [inline]
> >   dump_stack+0x191/0x1f0 lib/dump_stack.c:113
> >   panic+0x3c9/0xc1e kernel/panic.c:219
> >   kmsan_report+0x2a2/0x2b0 mm/kmsan/kmsan_report.c:131
> >   __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
> >   alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
> >   alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
> >   usb_stor_invoke_transport+0xf5/0x27e0
> > drivers/usb/storage/transport.c:606
> >   usb_stor_transparent_scsi_command+0x5d/0x70
> > drivers/usb/storage/protocol.c:108
> >   usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
> >   kthread+0x4b5/0x4f0 kernel/kthread.c:256
> >   ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
> > Kernel Offset: disabled
> > Rebooting in 86400 seconds..
> >
> >
> > ---
> > 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.
> > syzbot can test patches for this bug, for details see:
> > https://goo.gl/tpsmEJ#testing-patches
>
> #syz test: https://github.com/google/kmsan.git 1e76a3e5
This didn't work, let's try with the master:
#syz test: https://github.com/google/kmsan.git master

>
> diff --git a/drivers/usb/storage/alauda.c
> b/drivers/usb/storage/alauda.c
> index ddab2cd3d2e7..bb309b9ad65b 100644
> --- a/drivers/usb/storage/alauda.c
> +++ b/drivers/usb/storage/alauda.c
> @@ -452,7 +452,7 @@ static int alauda_init_media(struct us_data *us)
>  static int alauda_check_media(struct us_data *us)
>  {
>         struct alauda_info *info = (struct alauda_info *) us->extra;
> -       unsigned char status[2];
> +       unsigned char *status = us->iobuf;
>         int rc;
>
>         rc = alauda_get_media_status(us, status);
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/b8b1e4fef9f3ece63909c38b3302621d76770caa.camel%40gmail.com.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
  2019-10-11 11:23 ` Jaskaran Singh
  2019-10-11 11:51   ` Alexander Potapenko
@ 2019-10-11 14:08   ` Alan Stern
  2019-10-11 14:18     ` Andrey Konovalov
  2019-10-11 15:24   ` syzbot
  2 siblings, 1 reply; 11+ messages in thread
From: Alan Stern @ 2019-10-11 14:08 UTC (permalink / raw)
  To: Jaskaran Singh
  Cc: syzbot, glider, gregkh, linux-kernel, linux-usb, syzkaller-bugs,
	usb-storage

On Fri, 11 Oct 2019, Jaskaran Singh wrote:

> On Mon, 2019-10-07 at 12:39 -0700, syzbot wrote:
> > Hello,
> > 
> > syzbot found the following crash on:
> > 
> > HEAD commit:    1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with
> > kmsan_i..
> > git tree:       https://github.com/google/kmsan.git master
> > console output: 
> > https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
> > kernel config:  
> > https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
> > dashboard link: 
> > https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
> > compiler:       clang version 9.0.0 (/home/glider/llvm/clang  
> > 80fee25776c2fb61e74c1ecb1a523375c2500b69)
> > syz repro:      
> > https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
> > C reproducer:   
> > https://syzkaller.appspot.com/x/repro.c?x=110631b7600000
> > 
> > IMPORTANT: if you fix the bug, please add the following tag to the
> > commit:
> > Reported-by: syzbot+e7d46eb426883fb97efd@syzkaller.appspotmail.com
> > 
> > =====================================================
> > BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0  
> > drivers/usb/storage/alauda.c:1137
> > CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine,
> > BIOS  
> > Google 01/01/2011
> > Call Trace:
> >   __dump_stack lib/dump_stack.c:77 [inline]
> >   dump_stack+0x191/0x1f0 lib/dump_stack.c:113
> >   kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
> >   __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
> >   alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
> >   alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
> >   usb_stor_invoke_transport+0xf5/0x27e0
> > drivers/usb/storage/transport.c:606
> >   usb_stor_transparent_scsi_command+0x5d/0x70  
> > drivers/usb/storage/protocol.c:108
> >   usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
> >   kthread+0x4b5/0x4f0 kernel/kthread.c:256
> >   ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355


> #syz test: https://github.com/google/kmsan.git 1e76a3e5
> 
> diff --git a/drivers/usb/storage/alauda.c
> b/drivers/usb/storage/alauda.c
> index ddab2cd3d2e7..bb309b9ad65b 100644
> --- a/drivers/usb/storage/alauda.c
> +++ b/drivers/usb/storage/alauda.c
> @@ -452,7 +452,7 @@ static int alauda_init_media(struct us_data *us)
>  static int alauda_check_media(struct us_data *us)
>  {
>  	struct alauda_info *info = (struct alauda_info *) us->extra;
> -	unsigned char status[2];
> +	unsigned char *status = us->iobuf;
>  	int rc;
>  
>  	rc = alauda_get_media_status(us, status);

That is absolutely not the correct fix.

The problem is that after this call, the code does not check rc to see 
if an error occurred.  If there was an error, the value of status is 
meaningless so there's no point examining it at all.

Now yes, it's true that defining status as an array on the stack is 
also a bug, since USB transfer buffers are not allowed to be stack 
variables.  And the change you made _is_ the right way to fix that bug.  
But that is a separate bug, not the one that syzbot found.

Alan Stern


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
  2019-10-11 14:08   ` Alan Stern
@ 2019-10-11 14:18     ` Andrey Konovalov
  2019-10-11 14:53       ` Alan Stern
  0 siblings, 1 reply; 11+ messages in thread
From: Andrey Konovalov @ 2019-10-11 14:18 UTC (permalink / raw)
  To: Alan Stern
  Cc: Jaskaran Singh, syzbot, Alexander Potapenko, Greg Kroah-Hartman,
	LKML, USB list, syzkaller-bugs, usb-storage

On Fri, Oct 11, 2019 at 4:08 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, 11 Oct 2019, Jaskaran Singh wrote:
>
> > On Mon, 2019-10-07 at 12:39 -0700, syzbot wrote:
> > > Hello,
> > >
> > > syzbot found the following crash on:
> > >
> > > HEAD commit:    1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with
> > > kmsan_i..
> > > git tree:       https://github.com/google/kmsan.git master
> > > console output:
> > > https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
> > > kernel config:
> > > https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
> > > dashboard link:
> > > https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
> > > compiler:       clang version 9.0.0 (/home/glider/llvm/clang
> > > 80fee25776c2fb61e74c1ecb1a523375c2500b69)
> > > syz repro:
> > > https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
> > > C reproducer:
> > > https://syzkaller.appspot.com/x/repro.c?x=110631b7600000
> > >
> > > IMPORTANT: if you fix the bug, please add the following tag to the
> > > commit:
> > > Reported-by: syzbot+e7d46eb426883fb97efd@syzkaller.appspotmail.com
> > >
> > > =====================================================
> > > BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0
> > > drivers/usb/storage/alauda.c:1137
> > > CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine,
> > > BIOS
> > > Google 01/01/2011
> > > Call Trace:
> > >   __dump_stack lib/dump_stack.c:77 [inline]
> > >   dump_stack+0x191/0x1f0 lib/dump_stack.c:113
> > >   kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
> > >   __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
> > >   alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
> > >   alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
> > >   usb_stor_invoke_transport+0xf5/0x27e0
> > > drivers/usb/storage/transport.c:606
> > >   usb_stor_transparent_scsi_command+0x5d/0x70
> > > drivers/usb/storage/protocol.c:108
> > >   usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
> > >   kthread+0x4b5/0x4f0 kernel/kthread.c:256
> > >   ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
>
>
> > #syz test: https://github.com/google/kmsan.git 1e76a3e5
> >
> > diff --git a/drivers/usb/storage/alauda.c
> > b/drivers/usb/storage/alauda.c
> > index ddab2cd3d2e7..bb309b9ad65b 100644
> > --- a/drivers/usb/storage/alauda.c
> > +++ b/drivers/usb/storage/alauda.c
> > @@ -452,7 +452,7 @@ static int alauda_init_media(struct us_data *us)
> >  static int alauda_check_media(struct us_data *us)
> >  {
> >       struct alauda_info *info = (struct alauda_info *) us->extra;
> > -     unsigned char status[2];
> > +     unsigned char *status = us->iobuf;
> >       int rc;
> >
> >       rc = alauda_get_media_status(us, status);

[...]

> Now yes, it's true that defining status as an array on the stack is
> also a bug, since USB transfer buffers are not allowed to be stack
> variables.

Hi Alan,

I'm curious, what is the reason for disallowing that? Should we try to
somehow detect such cases automatically?

Thanks!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
  2019-10-11 14:18     ` Andrey Konovalov
@ 2019-10-11 14:53       ` Alan Stern
  2019-10-11 15:06         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 11+ messages in thread
From: Alan Stern @ 2019-10-11 14:53 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Jaskaran Singh, syzbot, Alexander Potapenko, Greg Kroah-Hartman,
	LKML, USB list, syzkaller-bugs, usb-storage

On Fri, 11 Oct 2019, Andrey Konovalov wrote:

> On Fri, Oct 11, 2019 at 4:08 PM Alan Stern <stern@rowland.harvard.edu> wrote:

> > Now yes, it's true that defining status as an array on the stack is
> > also a bug, since USB transfer buffers are not allowed to be stack
> > variables.
> 
> Hi Alan,
> 
> I'm curious, what is the reason for disallowing that? Should we try to
> somehow detect such cases automatically?

Transfer buffers are read and written by DMA.  On systems that don't
have cache-coherent DMA controllers, it is essential that the CPU does
not access any cache line involved in a DMA transfer while the transfer
is in progress.  Otherwise the data in the cache would be different
from the data in the buffer, leading to corruption.

(In theory it would be okay for the CPU to read (not write!) a cache
line assigned to a buffer for a DMA write (not read!) transfer.  But
even doing that isn't really a good idea.)

(Also, this isn't an issue for x86 architectures, because x86 has 
cache-coherent DMA.  But it is an issue on other architectures.)

In practice, this means transfer buffers have to be allocated by
something like kmalloc, so that they occupies their own separate set of
cache lines.  Buffers on the stack obviously don't satisfy this
requirement.

At some point there was a discussion about automatically detecting when
on-stack (or otherwise invalid) buffers are used for DMA transfers.  I
don't recall what the outcome was.

Alan Stern


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
  2019-10-11 14:53       ` Alan Stern
@ 2019-10-11 15:06         ` Greg Kroah-Hartman
  2019-10-14 12:56           ` Andrey Konovalov
  0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2019-10-11 15:06 UTC (permalink / raw)
  To: Alan Stern
  Cc: Andrey Konovalov, Jaskaran Singh, syzbot, Alexander Potapenko,
	LKML, USB list, syzkaller-bugs, usb-storage

On Fri, Oct 11, 2019 at 10:53:47AM -0400, Alan Stern wrote:
> On Fri, 11 Oct 2019, Andrey Konovalov wrote:
> 
> > On Fri, Oct 11, 2019 at 4:08 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > > Now yes, it's true that defining status as an array on the stack is
> > > also a bug, since USB transfer buffers are not allowed to be stack
> > > variables.
> > 
> > Hi Alan,
> > 
> > I'm curious, what is the reason for disallowing that? Should we try to
> > somehow detect such cases automatically?
> 
> Transfer buffers are read and written by DMA.  On systems that don't
> have cache-coherent DMA controllers, it is essential that the CPU does
> not access any cache line involved in a DMA transfer while the transfer
> is in progress.  Otherwise the data in the cache would be different
> from the data in the buffer, leading to corruption.
> 
> (In theory it would be okay for the CPU to read (not write!) a cache
> line assigned to a buffer for a DMA write (not read!) transfer.  But
> even doing that isn't really a good idea.)
> 
> (Also, this isn't an issue for x86 architectures, because x86 has 
> cache-coherent DMA.  But it is an issue on other architectures.)
> 
> In practice, this means transfer buffers have to be allocated by
> something like kmalloc, so that they occupies their own separate set of
> cache lines.  Buffers on the stack obviously don't satisfy this
> requirement.
> 
> At some point there was a discussion about automatically detecting when
> on-stack (or otherwise invalid) buffers are used for DMA transfers.  I
> don't recall what the outcome was.

A patchset from Kees was sent, but it needs a bit more work...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
  2019-10-11 11:23 ` Jaskaran Singh
  2019-10-11 11:51   ` Alexander Potapenko
  2019-10-11 14:08   ` Alan Stern
@ 2019-10-11 15:24   ` syzbot
  2 siblings, 0 replies; 11+ messages in thread
From: syzbot @ 2019-10-11 15:24 UTC (permalink / raw)
  To: glider, gregkh, jaskaransingh7654321, linux-kernel, linux-usb,
	stern, syzkaller-bugs, usb-storage

Hello,

syzbot has tested the proposed patch but the reproducer still triggered  
crash:
KMSAN: uninit-value in sd_revalidate_disk

=====================================================
BUG: KMSAN: uninit-value in check_disk_change+0x423/0x4b0  
fs/block_dev.c:1499
CPU: 1 PID: 23508 Comm: scsi_id Not tainted 5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
  media_not_present drivers/scsi/sd.c:1527 [inline]
  sd_spinup_disk drivers/scsi/sd.c:2096 [inline]
  sd_revalidate_disk+0x4d2/0xbef0 drivers/scsi/sd.c:3114
  check_disk_change+0x423/0x4b0 fs/block_dev.c:1499
  sd_open+0x471/0x8e0 drivers/scsi/sd.c:1356
  __blkdev_get+0x4a8/0x2480 fs/block_dev.c:1569
  blkdev_get+0x228/0x6d0 fs/block_dev.c:1707
  blkdev_open+0x36b/0x490 fs/block_dev.c:1846
  do_dentry_open+0xda7/0x1810 fs/open.c:797
  vfs_open+0xaf/0xe0 fs/open.c:906
  do_last fs/namei.c:3416 [inline]
  path_openat+0x17f4/0x6bb0 fs/namei.c:3533
  do_filp_open+0x2b8/0x710 fs/namei.c:3563
  do_sys_open+0x642/0xa30 fs/open.c:1089
  __do_sys_open fs/open.c:1107 [inline]
  __se_sys_open+0xad/0xc0 fs/open.c:1102
  __x64_sys_open+0x4a/0x70 fs/open.c:1102
  do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:297
  entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x7f7c9e529120
Code: 48 8b 15 1b 4d 2b 00 f7 d8 64 89 02 83 c8 ff c3 90 90 90 90 90 90 90  
90 90 90 83 3d d5 a4 2b 00 00 75 10 b8 02 00 00 00 0f 05 <48> 3d 01 f0 ff  
ff 73 31 c3 48 83 ec 08 e8 5e 8c 01 00 48 89 04 24
RSP: 002b:00007fff97dee0a8 EFLAGS: 00000246 ORIG_RAX: 0000000000000002
RAX: ffffffffffffffda RBX: 00007fff97dee5c0 RCX: 00007f7c9e529120
RDX: 00007fff97dee3c0 RSI: 0000000000000800 RDI: 00007fff97dee3c0
RBP: 00000000017ac010 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff97dee5c0
R13: 00007fff97dee3c0 R14: 00000000017ac010 R15: 0000000000000000

Local variable description: ----sshdr.i@sd_revalidate_disk
Variable was created at:
  sd_spinup_disk drivers/scsi/sd.c:3108 [inline]
  sd_revalidate_disk+0x2d3/0xbef0 drivers/scsi/sd.c:3114
  check_disk_change+0x423/0x4b0 fs/block_dev.c:1499
=====================================================


Tested on:

commit:         1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with kmsan_i..
git tree:       https://github.com/google/kmsan.git
console output: https://syzkaller.appspot.com/x/log.txt?x=144fd0a0e00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
dashboard link: https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler:       clang version 9.0.0 (/home/glider/llvm/clang  
80fee25776c2fb61e74c1ecb1a523375c2500b69)
patch:          https://syzkaller.appspot.com/x/patch.diff?x=110434ab600000


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
  2019-10-11 11:51   ` Alexander Potapenko
@ 2019-10-11 15:42     ` syzbot
  0 siblings, 0 replies; 11+ messages in thread
From: syzbot @ 2019-10-11 15:42 UTC (permalink / raw)
  To: glider, gregkh, jaskaransingh7654321, linux-kernel, linux-usb,
	stern, syzkaller-bugs, usb-storage

Hello,

syzbot has tested the proposed patch but the reproducer still triggered  
crash:
KMSAN: uninit-value in alauda_check_media

=====================================================
BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0  
drivers/usb/storage/alauda.c:1138
CPU: 1 PID: 11015 Comm: usb-storage Not tainted 5.4.0-rc2+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  kmsan_report+0x14c/0x2c0 mm/kmsan/kmsan_report.c:110
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:461
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1138
  usb_stor_invoke_transport+0xf5/0x27e0 drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355

Local variable description: ----status@alauda_check_media
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 11015 Comm: usb-storage Tainted: G    B             5.4.0-rc2+  
#0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  panic+0x3c9/0xc1e kernel/panic.c:220
  kmsan_report+0x2b4/0x2c0 mm/kmsan/kmsan_report.c:133
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:461
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1138
  usb_stor_invoke_transport+0xf5/0x27e0 drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Kernel Offset: disabled
Rebooting in 86400 seconds..


Tested on:

commit:         c40e5c97 kmsan: drop some dead code in kmsan_shadow.c
git tree:       https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=153ba453600000
kernel config:  https://syzkaller.appspot.com/x/.config?x=49548798e87d32d7
dashboard link: https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler:       clang version 9.0.0 (/home/glider/llvm/clang  
80fee25776c2fb61e74c1ecb1a523375c2500b69)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
  2019-10-11 15:06         ` Greg Kroah-Hartman
@ 2019-10-14 12:56           ` Andrey Konovalov
  0 siblings, 0 replies; 11+ messages in thread
From: Andrey Konovalov @ 2019-10-14 12:56 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Alan Stern, Jaskaran Singh, syzbot, Alexander Potapenko, LKML,
	USB list, syzkaller-bugs, usb-storage

On Fri, Oct 11, 2019 at 5:06 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Oct 11, 2019 at 10:53:47AM -0400, Alan Stern wrote:
> > On Fri, 11 Oct 2019, Andrey Konovalov wrote:
> >
> > > On Fri, Oct 11, 2019 at 4:08 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > > > Now yes, it's true that defining status as an array on the stack is
> > > > also a bug, since USB transfer buffers are not allowed to be stack
> > > > variables.
> > >
> > > Hi Alan,
> > >
> > > I'm curious, what is the reason for disallowing that? Should we try to
> > > somehow detect such cases automatically?
> >
> > Transfer buffers are read and written by DMA.  On systems that don't
> > have cache-coherent DMA controllers, it is essential that the CPU does
> > not access any cache line involved in a DMA transfer while the transfer
> > is in progress.  Otherwise the data in the cache would be different
> > from the data in the buffer, leading to corruption.
> >
> > (In theory it would be okay for the CPU to read (not write!) a cache
> > line assigned to a buffer for a DMA write (not read!) transfer.  But
> > even doing that isn't really a good idea.)
> >
> > (Also, this isn't an issue for x86 architectures, because x86 has
> > cache-coherent DMA.  But it is an issue on other architectures.)
> >
> > In practice, this means transfer buffers have to be allocated by
> > something like kmalloc, so that they occupies their own separate set of
> > cache lines.  Buffers on the stack obviously don't satisfy this
> > requirement.
> >
> > At some point there was a discussion about automatically detecting when
> > on-stack (or otherwise invalid) buffers are used for DMA transfers.  I
> > don't recall what the outcome was.
>
> A patchset from Kees was sent, but it needs a bit more work...

Hi Greg,

Could you send a link to the patchset?

Thanks!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: KMSAN: uninit-value in alauda_check_media
@ 2019-10-11 11:17 Jas K
  0 siblings, 0 replies; 11+ messages in thread
From: Jas K @ 2019-10-11 11:17 UTC (permalink / raw)
  To: syzbot+e7d46eb426883fb97efd
  Cc: stern, gregkh, linux-usb, usb-storage, linux-kernel

Hi, just taking a crack at this. Hope you guys don't mind.

#syz test: https://github.com/google/kasan.git 1e76a3e5

diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c
index ddab2cd3d2e7..bb309b9ad65b 100644
--- a/drivers/usb/storage/alauda.c
+++ b/drivers/usb/storage/alauda.c
@@ -452,7 +452,7 @@ static int alauda_init_media(struct us_data *us)
 static int alauda_check_media(struct us_data *us)
 {
 	struct alauda_info *info = (struct alauda_info *) us->extra;
-	unsigned char status[2];
+	unsigned char *status = us->iobuf;
 	int rc;
 
 	rc = alauda_get_media_status(us, status);

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07 19:39 KMSAN: uninit-value in alauda_check_media syzbot
2019-10-11 11:23 ` Jaskaran Singh
2019-10-11 11:51   ` Alexander Potapenko
2019-10-11 15:42     ` syzbot
2019-10-11 14:08   ` Alan Stern
2019-10-11 14:18     ` Andrey Konovalov
2019-10-11 14:53       ` Alan Stern
2019-10-11 15:06         ` Greg Kroah-Hartman
2019-10-14 12:56           ` Andrey Konovalov
2019-10-11 15:24   ` syzbot
2019-10-11 11:17 Jas K

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git