Linux-Wireless Archive on lore.kernel.org
 help / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: Christian Lamparter <chunkeey@gmail.com>
Cc: Oliver Neukum <oneukum@suse.com>,
	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 16:46:01 -0400 (EDT)
Message-ID: <Pine.LNX.4.44L0.1905171644110.1430-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <5014675.0cgHOJIxtM@debian64>

On Fri, 17 May 2019, Christian Lamparter wrote:

> 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) 

I agree; it doesn't seem to make sense.  The nice thing about syzbot, 
though, is you can ask it to run a debugging test for you.  Let's start 
by making sure that the faulty address really is &udev->dev.parent.

Alan


#syz test: https://github.com/google/kasan.git usb-fuzzer

 drivers/net/wireless/intersil/p54/p54usb.c |    3 +++
 1 file changed, 3 insertions(+)

Index: usb-devel/drivers/net/wireless/intersil/p54/p54usb.c
===================================================================
--- usb-devel.orig/drivers/net/wireless/intersil/p54/p54usb.c
+++ usb-devel/drivers/net/wireless/intersil/p54/p54usb.c
@@ -923,6 +923,7 @@ static void p54u_load_firmware_cb(const
 	struct usb_device *udev = priv->udev;
 	int err;
 
+	pr_info("%s: priv->udev = %px\n", __func__, udev);
 	complete(&priv->fw_wait_load);
 	if (firmware) {
 		priv->fw = firmware;
@@ -969,6 +970,8 @@ static int p54u_load_firmware(struct iee
 	if (i < 0)
 		return i;
 
+	dev_info(udev, "%s: udev @ %px, dev.parent @ %px\n",
+			__func__, udev, &udev->dev.parent);
 	dev_info(&priv->udev->dev, "Loading firmware file %s\n",
 	       p54u_fwlist[i].fw);
 


  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
2019-05-17 20:46       ` Alan Stern [this message]
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=Pine.LNX.4.44L0.1905171644110.1430-100000@iolanthe.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=andreyknvl@google.com \
    --cc=chunkeey@gmail.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