netdev - Re: KASAN: use-after-free Read in p54u_load_firmware

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 18 May 2019 11:13:39 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     syzbot <syzbot+200d4bb11b23d929335f@...kaller.appspotmail.com>
cc:     andreyknvl@...gle.com, <chunkeey@...il.com>,
        <chunkeey@...glemail.com>, <davem@...emloft.net>,
        <kvalo@...eaurora.org>,
        Kernel development list <linux-kernel@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>,
        <linux-wireless@...r.kernel.org>, <netdev@...r.kernel.org>,
        Oliver Neukum <oneukum@...e.com>,
        <syzkaller-bugs@...glegroups.com>
Subject: Re: KASAN: use-after-free Read in p54u_load_firmware_cb

On Fri, 17 May 2019, syzbot wrote:

> Hello,
> 
> syzbot tried to test the proposed patch but build/boot failed:

Drat.  Mistake in the patch.  Let's try again.

Incidentally, as far as I can tell there's no point in having the
usb_get_dev() in p54u_probe() and usb_put_dev() in p54u_disconnect().  
The device structure is guaranteed not to be deallocated while a driver
is bound to any of its interfaces, so taking an extra reference won't
make any difference.

On the other hand, I do see some problems in the firmware-load
callback.  First, it calls device_release_driver() without first
checking that the interface is still bound to the p54u driver.  
Second, it shouldn't call device_release_driver() at all -- it should
call usb_driver_release_interface().  It doesn't want to unbind the USB
device's driver; it wants to unbind the interface's driver.  And third,
to do this it needs to acquire udev's device lock, not the lock for
udev's parent.

Alan Stern

#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->dev, "%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);