Linux-Wireless Archive on lore.kernel.org
 help / Atom feed
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> (raw)
In-Reply-To: <1557754110.2793.7.camel@suse.com>

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



  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-06 11:16 syzbot
2019-05-13 10:23 ` syzbot
2019-05-13 13:28   ` Oliver Neukum
2019-05-17 19:21     ` Christian Lamparter [this message]
2019-05-17 20:46       ` Alan Stern
2019-05-17 21:01         ` syzbot
2019-05-18 15:13           ` Alan Stern
2019-05-18 15:50             ` syzbot
2019-05-18 16:32               ` Alan Stern
2019-05-18 16:50                 ` syzbot
2019-05-18 17:01                   ` Alan Stern
2019-05-18 17:36                     ` syzbot
2019-05-18 17:49                       ` Alan Stern
2019-05-18 18:31                         ` syzbot
2019-05-18 20:11                         ` Christian Lamparter
2019-05-20 14:44       ` [PATCH] network: wireless: p54u: Fix race between disconnect and firmware loading Alan Stern
2019-05-24 21:19         ` Christian Lamparter

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5014675.0cgHOJIxtM@debian64 \
    --to=chunkeey@gmail.com \
    --cc=andreyknvl@google.com \
    --cc=chunkeey@googlemail.com \
    --cc=davem@davemloft.net \
    --cc=flamingice@sourmilk.net \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=oneukum@suse.com \
    --cc=syzbot+200d4bb11b23d929335f@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/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-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org linux-wireless@archiver.kernel.org
	public-inbox-index linux-wireless


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


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