Re: KASAN: use-after-free Read in p54u_load_firmware_cb
From: Alan Stern
Date: Sat May 18 2019 - 12:34:25 EST
On Sat, 18 May 2019, syzbot wrote:
> Hello,
>
> syzbot has tested the proposed patch but the reproducer still triggered
> crash:
> KASAN: slab-out-of-bounds Read in p54u_load_firmware_cb
>
> usb 6-1: Direct firmware load for isl3887usb failed with error -2
> p54u_load_firmware_cb: priv->udev = ffff88809ad5bb80
> usb 6-1: Firmware not found.
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in p54u_load_firmware_cb+0x3c9/0x45f
> drivers/net/wireless/intersil/p54/p54usb.c:937
> Read of size 8 at addr ffff88809abab588 by task kworker/1:8/5526
If you look at the full console log, you'll see that this address is
different from all the addresses printed out earlier. Not what you
would expect.
Which makes me wonder: Suppose a disconnect occurs before the firmware
loader callback runs? p54_disconnect() will get stuck waiting for the
priv->fw_load_wait completion to fire. But when the callback runs, the
first thing it does is activate the completion.
So now p54_disconnect() can finish doing its thing, including
deallocating all the private data structures. Then when
p54u_load_firmware_cb() tries to read the contents of priv, it ends up
accessing deallocated memory!
The patch below tries to do things the right way.
Alan Stern
#syz test: https://github.com/google/kasan.git usb-fuzzer
drivers/net/wireless/intersil/p54/p54usb.c | 37 +++++++++++------------------
1 file changed, 15 insertions(+), 22 deletions(-)
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
@@ -33,6 +33,8 @@ MODULE_ALIAS("prism54usb");
MODULE_FIRMWARE("isl3886usb");
MODULE_FIRMWARE("isl3887usb");
+static struct usb_driver p54u_driver;
+
/*
* Note:
*
@@ -921,9 +923,9 @@ static void p54u_load_firmware_cb(const
{
struct p54u_priv *priv = context;
struct usb_device *udev = priv->udev;
+ struct usb_interface *intf = priv->intf;
int err;
- complete(&priv->fw_wait_load);
if (firmware) {
priv->fw = firmware;
err = p54u_start_ops(priv);
@@ -932,23 +934,19 @@ static void p54u_load_firmware_cb(const
dev_err(&udev->dev, "Firmware not found.\n");
}
- if (err) {
- struct device *parent = priv->udev->dev.parent;
-
- dev_err(&udev->dev, "failed to initialize device (%d)\n", err);
-
- if (parent)
- device_lock(parent);
+ complete(&priv->fw_wait_load);
+ /*
+ * At this point p54u_disconnect may have already freed
+ * the "priv" context. Do not use it anymore!
+ */
+ priv = NULL;
- device_release_driver(&udev->dev);
- /*
- * At this point p54u_disconnect has already freed
- * the "priv" context. Do not use it anymore!
- */
- priv = NULL;
+ if (err) {
+ dev_err(&intf->dev, "failed to initialize device (%d)\n", err);
- if (parent)
- device_unlock(parent);
+ usb_lock_device(udev);
+ usb_driver_release_interface(intf, &p54u_driver);
+ usb_unlock_device(udev);
}
usb_put_dev(udev);
@@ -1011,8 +1009,6 @@ static int p54u_probe(struct usb_interfa
skb_queue_head_init(&priv->rx_queue);
init_usb_anchor(&priv->submitted);
- usb_get_dev(udev);
-
/* really lazy and simple way of figuring out if we're a 3887 */
/* TODO: should just stick the identification in the device table */
i = intf->altsetting->desc.bNumEndpoints;
@@ -1053,10 +1049,8 @@ static int p54u_probe(struct usb_interfa
priv->upload_fw = p54u_upload_firmware_net2280;
}
err = p54u_load_firmware(dev, intf);
- if (err) {
- usb_put_dev(udev);
+ if (err)
p54_free_common(dev);
- }
return err;
}
@@ -1072,7 +1066,6 @@ static void p54u_disconnect(struct usb_i
wait_for_completion(&priv->fw_wait_load);
p54_unregister_common(dev);
- usb_put_dev(interface_to_usbdev(intf));
release_firmware(priv->fw);
p54_free_common(dev);
}