Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Christian Lamparter <chunkeey@gmail.com>
To:     Oliver Neukum <oneukum@suse.com>
Cc:     syzbot <syzbot+200d4bb11b23d929335f@syzkaller.appspotmail.com>,
        kvalo@codeaurora.org, davem@davemloft.net, andreyknvl@google.com,
        syzkaller-bugs@googlegroups.com, chunkeey@googlemail.com,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        Michael Wu <flamingice@sourmilk.net>
Subject: Re: KASAN: use-after-free Read in p54u_load_firmware_cb
Date:   Fri, 17 May 2019 21:21:27 +0200
Message-ID: <5014675.0cgHOJIxtM@debian64>
In-Reply-To: <1557754110.2793.7.camel@suse.com>
References: <00000000000073512b0588c24d09@google.com> <1557754110.2793.7.camel@suse.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Monday, May 13, 2019 3:28:30 PM CEST Oliver Neukum wrote:
> On Mo, 2019-05-13 at 03:23 -0700, syzbot wrote:
> > syzbot has found a reproducer for the following crash on:
> > 
> > HEAD commit:    43151d6c usb-fuzzer: main usb gadget fuzzer driver
> > git tree:       https://github.com/google/kasan.git usb-fuzzer
> > console output: https://syzkaller.appspot.com/x/log.txt?x=16b64110a00000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=4183eeef650d1234
> > dashboard link: https://syzkaller.appspot.com/bug?extid=200d4bb11b23d929335f
> > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1634c900a00000
> > 
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+200d4bb11b23d929335f@syzkaller.appspotmail.com
> > 
> > usb 1-1: config 0 descriptor??
> > usb 1-1: reset high-speed USB device number 2 using dummy_hcd
> > usb 1-1: device descriptor read/64, error -71
> > usb 1-1: Using ep0 maxpacket: 8
> > usb 1-1: Loading firmware file isl3887usb
> > usb 1-1: Direct firmware load for isl3887usb failed with error -2
> > usb 1-1: Firmware not found.
> > ==================================================================
> > BUG: KASAN: use-after-free in p54u_load_firmware_cb.cold+0x97/0x13a  
> > drivers/net/wireless/intersil/p54/p54usb.c:936
> > Read of size 8 at addr ffff88809803f588 by task kworker/1:0/17
> 
> Hi,
> 
> it looks to me as if refcounting is broken.
> You should have a usb_put_dev() in p54u_load_firmware_cb() or in
> p54u_disconnect(), but not both.

There's more to that refcounting that meets the eye. Do you see that
request_firmware_nowait() in the driver? That's the async firmware
request call that get's completed by the p54u_load_firmware_cb()
So what's happening here is that the driver has to be protected
against rmmod when the driver is waiting for request_firmware_nowait
to "finally" callback, which depending on the system can be up to 
60 seconds.

Now, what seems to be odd is that it's at line 936
> > BUG: KASAN: use-after-free in p54u_load_firmware_cb.cold+0x97/0x13a  
> > drivers/net/wireless/intersil/p54/p54usb.c:936

because if you put it in context:

|
|static void p54u_load_firmware_cb(const struct firmware *firmware,
|				  void *context)
|{
|	struct p54u_priv *priv = context;
|	struct usb_device *udev = priv->udev;
|	int err;
|
|	complete(&priv->fw_wait_load);
|	if (firmware) {
|		priv->fw = firmware;
|		err = p54u_start_ops(priv);
|	} else {
|		err = -ENOENT;
|		dev_err(&udev->dev, "Firmware not found.\n");
|	}
|
|	if (err) {
|>>	>>	struct device *parent = priv->udev->dev.parent; <<<<-- 936 is here
|
|		dev_err(&udev->dev, "failed to initialize device (%d)\n", err);
|
|		if (parent)
|			device_lock(parent);
|
|		device_release_driver(&udev->dev);
|		/*
|		 * At this point p54u_disconnect has already freed
|		 * the "priv" context. Do not use it anymore!
|		 */
|		priv = NULL;
|
|		if (parent)
|			device_unlock(parent);
|	}
|
|	usb_put_dev(udev);
|}

it seems very out of place, because at that line the device is still bound to
the driver! Only with device_release_driver in line 942, I could see that
something woulb be aray... !BUT! that's why we do have the extra
usb_get_dev(udev) in p54u_load_firmware() so we can do the usb_put_dev(udev) in
line 953 to ensure that nothing (like the rmmod I talked above) will interfere
until everything is done.

I've no idea what's wrong here, is gcc 9.0 aggressivly reording the put? Or is
something else going on with the sanitizers? Because this report does look
dogdy there!

(Note: p54usb has !strategic! dev_err/infos in place right around the
usb_get_dev/usb_put_dev so we can sort of tell the refvalue of the udev
and it all seems to be correct from what I can gleam) 

Regards,
Christian