Re: KASAN: use-after-free Read in dvb_usb_device_exit

[Oliver Neukum]

On Mi, 2019-04-24 at 16:09 +0200, Hans Verkuil wrote:
> On 4/15/19 1:12 PM, Oliver Neukum wrote:
> > On Fr, 2019-04-12 at 04:46 -0700, syzbot wrote:
> > > Hello,
> > > 
> > > syzbot found the following crash on:
> > > 
> > > HEAD commit:    9a33b369 usb-fuzzer: main usb gadget fuzzer driver
> > > git tree:       https://github.com/google/kasan/tree/usb-fuzzer
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=1643974b200000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=23e37f59d94ddd15
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=26ec41e9f788b3eba396
> > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12f5efa7200000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1395a0f3200000
> > > 
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+26ec41e9f788b3eba396@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > 
> > > dvb-usb: schedule remote query interval to 150 msecs.
> > > dw2102: su3000_power_ctrl: 0, initialized 1
> > > dvb-usb: TeVii S421 PCI successfully initialized and connected.
> > > usb 1-1: USB disconnect, device number 2
> > 
> > Hi,
> > 
> > proposed fix. If nobody objects, I will submit it.
> > 
> > 	Regards
> > 		Oliver
> > 
> > From d6097d205ac61745334b79639d3b8b910ae66c71 Mon Sep 17 00:00:00 2001
> > From: Oliver Neukum <oneukum@xxxxxxxx>
> > Date: Mon, 15 Apr 2019 13:06:01 +0200
> > Subject: [PATCH] dvb: usb: fix use after free in dvb_usb_device_exit
> > 
> > dvb_usb_device_exit() frees and uses teh device name in that order
> > Fix by storing the name in a buffer before freeing it
> > 
> > Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
> > Reported-by: syzbot+26ec41e9f788b3eba396@xxxxxxxxxxxxxxxxxxxxxxxxx
> > ---
> >  drivers/media/usb/dvb-usb/dvb-usb-init.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> > index 99951e02a880..2e1670cc3903 100644
> > --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c
> > +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c
> > @@ -288,13 +288,18 @@ void dvb_usb_device_exit(struct usb_interface *intf)
> >  {
> >  	struct dvb_usb_device *d = usb_get_intfdata(intf);
> >  	const char *name = "generic DVB-USB module";
> > +	char identifier[40];
> >  
> >  	usb_set_intfdata(intf, NULL);
> >  	if (d != NULL && d->desc != NULL) {
> >  		name = d->desc->name;
> > +		memcpy(identifier, name, 39);
> > +		identifier[39] = NULL;
> >  		dvb_usb_exit(d);
> 
> Why not just move this to after the info()? You'll need to repeat the
> 'if' in that case, but that way there is no need to memcpy anything.

The info() would make the incorrect claim that something has been
freed. It looks to me like it exists to guarantee that you know that
nothing hung while freeing.

	Regards
		Oliver