Re: KASAN: use-after-free Read in usbhid_close (3)

From: Alan Stern
Date: Fri Apr 17 2020 - 15:15:37 EST


On Sun, 12 Apr 2020, syzbot wrote:

> syzbot has found a reproducer for the following crash on:
>
> HEAD commit: 0fa84af8 Merge tag 'usb-serial-5.7-rc1' of https://git.ker..
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=14453a8be00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=6b9c154b0c23aecf
> dashboard link: https://syzkaller.appspot.com/bug?extid=7bf5a7b0f0a1f9446f4c
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=140c644fe00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=163fb28be00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+7bf5a7b0f0a1f9446f4c@xxxxxxxxxxxxxxxxxxxxxxxxx
>
> ==================================================================
> BUG: KASAN: use-after-free in atomic_read include/asm-generic/atomic-instrumented.h:26 [inline]
> BUG: KASAN: use-after-free in usb_kill_urb drivers/usb/core/urb.c:696 [inline]
> BUG: KASAN: use-after-free in usb_kill_urb+0x24b/0x2c0 drivers/usb/core/urb.c:688
> Read of size 4 at addr ffff8881c6d6e210 by task systemd-udevd/1137

Details removed. Given how hard this is to reproduce, it definitely
seems like some sort of race. But it's very hard to tell what is
racing with what.

Let's start off easy and get a little extra information at the point
where the bug occurs. As far as I can tell, usbhid_close() is always
supposed to be called before usbhid_stop().

Alan Stern

#syz test: https://github.com/google/kasan.git 0fa84af8

Index: usb-devel/drivers/hid/usbhid/hid-core.c
===================================================================
--- usb-devel.orig/drivers/hid/usbhid/hid-core.c
+++ usb-devel/drivers/hid/usbhid/hid-core.c
@@ -747,6 +747,13 @@ static void usbhid_close(struct hid_devi
return;

hid_cancel_delayed_stuff(usbhid);
+ if (usbhid->alan_alloc == 0)
+ dev_WARN(&usbhid->intf->dev, "Close after dealloc (open %d)\n",
+ usbhid->alan_open);
+ if (usbhid->alan_open != 1)
+ dev_WARN(&usbhid->intf->dev, "Close while open = %d\n",
+ usbhid->alan_open);
+ --usbhid->alan_open;
usb_kill_urb(usbhid->urbin);
usbhid->intf->needs_remote_wakeup = 0;
}
@@ -1120,6 +1127,7 @@ static int usbhid_start(struct hid_devic
continue;
if (!(usbhid->urbin = usb_alloc_urb(0, GFP_KERNEL)))
goto fail;
+ ++usbhid->alan_alloc;
pipe = usb_rcvintpipe(dev, endpoint->bEndpointAddress);
usb_fill_int_urb(usbhid->urbin, dev, pipe, usbhid->inbuf, insize,
hid_irq_in, hid, interval);
@@ -1177,6 +1185,7 @@ static int usbhid_start(struct hid_devic
usbhid_set_leds(hid);
device_set_wakeup_enable(&dev->dev, 1);
}
+ ++usbhid->alan_open;
return 0;

fail:
@@ -1197,6 +1206,9 @@ static void usbhid_stop(struct hid_devic
if (WARN_ON(!usbhid))
return;

+ if (usbhid->alan_open > 0)
+ dev_WARN(&usbhid->intf->dev, "Stop while open (alloc = %d)\n",
+ usbhid->alan_alloc);
if (hid->quirks & HID_QUIRK_ALWAYS_POLL) {
clear_bit(HID_IN_POLLING, &usbhid->iofl);
usbhid->intf->needs_remote_wakeup = 0;
@@ -1206,6 +1218,7 @@ static void usbhid_stop(struct hid_devic
spin_lock_irq(&usbhid->lock); /* Sync with error and led handlers */
set_bit(HID_DISCONNECTED, &usbhid->iofl);
spin_unlock_irq(&usbhid->lock);
+ --usbhid->alan_alloc;
usb_kill_urb(usbhid->urbin);
usb_kill_urb(usbhid->urbout);
usb_kill_urb(usbhid->urbctrl);
Index: usb-devel/drivers/hid/usbhid/usbhid.h
===================================================================
--- usb-devel.orig/drivers/hid/usbhid/usbhid.h
+++ usb-devel/drivers/hid/usbhid/usbhid.h
@@ -87,6 +87,9 @@ struct usbhid_device {
unsigned int retry_delay; /* Delay length in ms */
struct work_struct reset_work; /* Task context for resets */
wait_queue_head_t wait; /* For sleeping */
+
+ int alan_alloc;
+ int alan_open;
};

#define hid_to_usb_dev(hid_dev) \