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@xxxxxxxxxxxxxxxxxxxxxxxxx > > > > > > 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);
