Re: KMSAN: uninit-value in alauda_check_media

[Alan Stern]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > 
> > =====================================================
> > 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