Hi!
It seems that gadget->ops can be NULL so it probably needs to be
checked as well as gadget->ops->ioctl in dev_ioctl() in
drivers/usb/gadget/legacy/inode.c.
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN
Modules linked in:
CPU: 1 PID: 5214 Comm: syz-executor Not tainted 4.13.0+ #94
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff88006ac0e800 task.stack: ffff88006af60000
! handling hub events now: start
RIP: 0010:dev_ioctl+0x117/0x280 drivers/usb/gadget/legacy/inode.c:1323
RSP: 0018:ffff88006af67c30 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: 1ffff1000d5ecf86 RCX: ffffc90000ab5000
RDX: 0000000000000005 RSI: ffffffff83f1c482 RDI: 0000000000000028
RBP: ffff88006af67cf8 R08: ffffed000d5ecf80 R09: ffffed000d5ecf81
R10: ffff88006af67ed8 R11: 0000000000000000 R12: 0000000000000414
R13: 0000000000000000 R14: dffffc0000000000 R15: 0000000000000000
FS: 00007fd2fcbc1700(0000) GS:ffff88006c900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000006e30a0 CR3: 000000006a3ef000 CR4: 00000000000006e0
Call Trace:
vfs_ioctl fs/ioctl.c:45
do_vfs_ioctl+0x1d2/0x1560 fs/ioctl.c:685
SYSC_ioctl fs/ioctl.c:700
SyS_ioctl+0x94/0xc0 fs/ioctl.c:691
entry_SYSCALL_64_fastpath+0x1a/0xa5 arch/x86/entry/entry_64.S:203
RIP: 0033:0x447727
RSP: 002b:00007fd2fcbc0bd8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000708000 RCX: 0000000000447727
RDX: 0000000000000000 RSI: 0000000000000414 RDI: 0000000000000005
RBP: 0000000000005da0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000206 R12: 0000000000000005
R13: 00000000ffffffff R14: 000000000000004c R15: 0000000020003000
Code: 80 96 50 fd 41 81 fc 42 42 00 00 0f 84 b3 00 00 00 e8 6e 96 50
fd 49 8d 7f 28 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <80>
3c 02 00 0f 85 4e 01 00 00 4d 8b 77 28 48 b8 00 00 00 00 00
RIP: dev_ioctl+0x117/0x280 RSP: ffff88006af67c30
---[ end trace 2fb9696e75c067ba ]---
On Mon, 11 Sep 2017, Andrey Konovalov wrote:
> Hi!
>
> It seems that gadget->ops can be NULL so it probably needs to be
> checked as well as gadget->ops->ioctl in dev_ioctl() in
> drivers/usb/gadget/legacy/inode.c.
Actually, I suspect the problem is that gadget is NULL, not
gadget->ops.
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] SMP KASAN
> Modules linked in:
> CPU: 1 PID: 5214 Comm: syz-executor Not tainted 4.13.0+ #94
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: ffff88006ac0e800 task.stack: ffff88006af60000
> ! handling hub events now: start
> RIP: 0010:dev_ioctl+0x117/0x280 drivers/usb/gadget/legacy/inode.c:1323
Can you test the patch below? I haven't tried it myself yet, but I
think it will fix the problem you found.
Alan Stern
Index: usb-4.x/drivers/usb/gadget/legacy/inode.c
===================================================================
--- usb-4.x.orig/drivers/usb/gadget/legacy/inode.c
+++ usb-4.x/drivers/usb/gadget/legacy/inode.c
@@ -28,7 +28,7 @@
#include <linux/aio.h>
#include <linux/uio.h>
#include <linux/refcount.h>
-
+#include <linux/delay.h>
#include <linux/device.h>
#include <linux/moduleparam.h>
@@ -116,6 +116,7 @@ enum ep0_state {
struct dev_data {
spinlock_t lock;
refcount_t count;
+ int udc_usage;
enum ep0_state state; /* P: lock */
struct usb_gadgetfs_event event [N_EVENT];
unsigned ev_next;
@@ -513,9 +514,9 @@ static void ep_aio_complete(struct usb_e
INIT_WORK(&priv->work, ep_user_copy_worker);
schedule_work(&priv->work);
}
- spin_unlock(&epdata->dev->lock);
usb_ep_free_request(ep, req);
+ spin_unlock(&epdata->dev->lock);
put_ep(epdata);
}
@@ -939,9 +940,11 @@ ep0_read (struct file *fd, char __user *
struct usb_request *req = dev->req;
if ((retval = setup_req (ep, req, 0)) == 0) {
+ ++dev->udc_usage;
spin_unlock_irq (&dev->lock);
retval = usb_ep_queue (ep, req, GFP_KERNEL);
spin_lock_irq (&dev->lock);
+ --dev->udc_usage;
}
dev->state = STATE_DEV_CONNECTED;
@@ -1131,6 +1134,7 @@ ep0_write (struct file *fd, const char _
retval = setup_req (dev->gadget->ep0, dev->req, len);
if (retval == 0) {
dev->state = STATE_DEV_CONNECTED;
+ ++dev->udc_usage;
spin_unlock_irq (&dev->lock);
if (copy_from_user (dev->req->buf, buf, len))
retval = -EFAULT;
@@ -1142,6 +1146,7 @@ ep0_write (struct file *fd, const char _
GFP_KERNEL);
}
spin_lock_irq(&dev->lock);
+ --dev->udc_usage;
if (retval < 0) {
clean_req (dev->gadget->ep0, dev->req);
} else
@@ -1243,9 +1248,21 @@ static long dev_ioctl (struct file *fd,
struct usb_gadget *gadget = dev->gadget;
long ret = -ENOTTY;
- if (gadget->ops->ioctl)
+ spin_lock_irq(&dev->lock);
+ if (dev->state == STATE_DEV_OPENED ||
+ dev->state == STATE_DEV_UNBOUND) {
+ /* Not bound to a UDC */
+ } else if (gadget->ops->ioctl) {
+ ++dev->udc_usage;
+ spin_unlock_irq(&dev->lock);
+
ret = gadget->ops->ioctl (gadget, code, value);
+ spin_lock_irq(&dev->lock);
+ --dev->udc_usage;
+ }
+ spin_unlock_irq(&dev->lock);
+
return ret;
}
@@ -1463,10 +1480,12 @@ delegate:
if (value < 0)
break;
+ ++dev->udc_usage;
spin_unlock (&dev->lock);
value = usb_ep_queue (gadget->ep0, dev->req,
GFP_KERNEL);
spin_lock (&dev->lock);
+ --dev->udc_usage;
if (value < 0) {
clean_req (gadget->ep0, dev->req);
break;
@@ -1490,8 +1509,12 @@ delegate:
req->length = value;
req->zero = value < w_length;
+ ++dev->udc_usage;
spin_unlock (&dev->lock);
value = usb_ep_queue (gadget->ep0, req, GFP_KERNEL);
+ spin_lock(&dev->lock);
+ --dev->udc_usage;
+ spin_unlock(&dev->lock);
if (value < 0) {
DBG (dev, "ep_queue --> %d\n", value);
req->status = 0;
@@ -1518,21 +1541,24 @@ static void destroy_ep_files (struct dev
/* break link to FS */
ep = list_first_entry (&dev->epfiles, struct ep_data, epfiles);
list_del_init (&ep->epfiles);
+ spin_unlock_irq (&dev->lock);
+
dentry = ep->dentry;
ep->dentry = NULL;
parent = d_inode(dentry->d_parent);
/* break link to controller */
+ mutex_lock(&ep->lock);
if (ep->state == STATE_EP_ENABLED)
(void) usb_ep_disable (ep->ep);
ep->state = STATE_EP_UNBOUND;
usb_ep_free_request (ep->ep, ep->req);
ep->ep = NULL;
+ mutex_unlock(&ep->lock);
+
wake_up (&ep->wait);
put_ep (ep);
- spin_unlock_irq (&dev->lock);
-
/* break link to dcache */
inode_lock(parent);
d_delete (dentry);
@@ -1603,6 +1629,11 @@ gadgetfs_unbind (struct usb_gadget *gadg
spin_lock_irq (&dev->lock);
dev->state = STATE_DEV_UNBOUND;
+ while (dev->udc_usage > 0) {
+ spin_unlock_irq(&dev->lock);
+ usleep_range(1000, 2000);
+ spin_lock_irq(&dev->lock);
+ }
spin_unlock_irq (&dev->lock);
destroy_ep_files (dev);
On Wed, Sep 20, 2017 at 7:59 PM, Alan Stern <[email protected]> wrote:
> On Mon, 11 Sep 2017, Andrey Konovalov wrote:
>
>> Hi!
>>
>> It seems that gadget->ops can be NULL so it probably needs to be
>> checked as well as gadget->ops->ioctl in dev_ioctl() in
>> drivers/usb/gadget/legacy/inode.c.
>
> Actually, I suspect the problem is that gadget is NULL, not
> gadget->ops.
Yes, you are correct, checked it by adding printk. Attaching a simple
repro of the issue just for reference.
>
>> kasan: CONFIG_KASAN_INLINE enabled
>> kasan: GPF could be caused by NULL-ptr deref or user memory access
>> general protection fault: 0000 [#1] SMP KASAN
>> Modules linked in:
>> CPU: 1 PID: 5214 Comm: syz-executor Not tainted 4.13.0+ #94
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> task: ffff88006ac0e800 task.stack: ffff88006af60000
>> ! handling hub events now: start
>> RIP: 0010:dev_ioctl+0x117/0x280 drivers/usb/gadget/legacy/inode.c:1323
>
> Can you test the patch below? I haven't tried it myself yet, but I
> think it will fix the problem you found.
This fixes the crash for me.
Tested-by: Andrey Konovalov <[email protected]>
Thanks!
>
> Alan Stern
>
>
>
> Index: usb-4.x/drivers/usb/gadget/legacy/inode.c
> ===================================================================
> --- usb-4.x.orig/drivers/usb/gadget/legacy/inode.c
> +++ usb-4.x/drivers/usb/gadget/legacy/inode.c
> @@ -28,7 +28,7 @@
> #include <linux/aio.h>
> #include <linux/uio.h>
> #include <linux/refcount.h>
> -
> +#include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/moduleparam.h>
>
> @@ -116,6 +116,7 @@ enum ep0_state {
> struct dev_data {
> spinlock_t lock;
> refcount_t count;
> + int udc_usage;
> enum ep0_state state; /* P: lock */
> struct usb_gadgetfs_event event [N_EVENT];
> unsigned ev_next;
> @@ -513,9 +514,9 @@ static void ep_aio_complete(struct usb_e
> INIT_WORK(&priv->work, ep_user_copy_worker);
> schedule_work(&priv->work);
> }
> - spin_unlock(&epdata->dev->lock);
>
> usb_ep_free_request(ep, req);
> + spin_unlock(&epdata->dev->lock);
> put_ep(epdata);
> }
>
> @@ -939,9 +940,11 @@ ep0_read (struct file *fd, char __user *
> struct usb_request *req = dev->req;
>
> if ((retval = setup_req (ep, req, 0)) == 0) {
> + ++dev->udc_usage;
> spin_unlock_irq (&dev->lock);
> retval = usb_ep_queue (ep, req, GFP_KERNEL);
> spin_lock_irq (&dev->lock);
> + --dev->udc_usage;
> }
> dev->state = STATE_DEV_CONNECTED;
>
> @@ -1131,6 +1134,7 @@ ep0_write (struct file *fd, const char _
> retval = setup_req (dev->gadget->ep0, dev->req, len);
> if (retval == 0) {
> dev->state = STATE_DEV_CONNECTED;
> + ++dev->udc_usage;
> spin_unlock_irq (&dev->lock);
> if (copy_from_user (dev->req->buf, buf, len))
> retval = -EFAULT;
> @@ -1142,6 +1146,7 @@ ep0_write (struct file *fd, const char _
> GFP_KERNEL);
> }
> spin_lock_irq(&dev->lock);
> + --dev->udc_usage;
> if (retval < 0) {
> clean_req (dev->gadget->ep0, dev->req);
> } else
> @@ -1243,9 +1248,21 @@ static long dev_ioctl (struct file *fd,
> struct usb_gadget *gadget = dev->gadget;
> long ret = -ENOTTY;
>
> - if (gadget->ops->ioctl)
> + spin_lock_irq(&dev->lock);
> + if (dev->state == STATE_DEV_OPENED ||
> + dev->state == STATE_DEV_UNBOUND) {
> + /* Not bound to a UDC */
> + } else if (gadget->ops->ioctl) {
> + ++dev->udc_usage;
> + spin_unlock_irq(&dev->lock);
> +
> ret = gadget->ops->ioctl (gadget, code, value);
>
> + spin_lock_irq(&dev->lock);
> + --dev->udc_usage;
> + }
> + spin_unlock_irq(&dev->lock);
> +
> return ret;
> }
>
> @@ -1463,10 +1480,12 @@ delegate:
> if (value < 0)
> break;
>
> + ++dev->udc_usage;
> spin_unlock (&dev->lock);
> value = usb_ep_queue (gadget->ep0, dev->req,
> GFP_KERNEL);
> spin_lock (&dev->lock);
> + --dev->udc_usage;
> if (value < 0) {
> clean_req (gadget->ep0, dev->req);
> break;
> @@ -1490,8 +1509,12 @@ delegate:
> req->length = value;
> req->zero = value < w_length;
>
> + ++dev->udc_usage;
> spin_unlock (&dev->lock);
> value = usb_ep_queue (gadget->ep0, req, GFP_KERNEL);
> + spin_lock(&dev->lock);
> + --dev->udc_usage;
> + spin_unlock(&dev->lock);
> if (value < 0) {
> DBG (dev, "ep_queue --> %d\n", value);
> req->status = 0;
> @@ -1518,21 +1541,24 @@ static void destroy_ep_files (struct dev
> /* break link to FS */
> ep = list_first_entry (&dev->epfiles, struct ep_data, epfiles);
> list_del_init (&ep->epfiles);
> + spin_unlock_irq (&dev->lock);
> +
> dentry = ep->dentry;
> ep->dentry = NULL;
> parent = d_inode(dentry->d_parent);
>
> /* break link to controller */
> + mutex_lock(&ep->lock);
> if (ep->state == STATE_EP_ENABLED)
> (void) usb_ep_disable (ep->ep);
> ep->state = STATE_EP_UNBOUND;
> usb_ep_free_request (ep->ep, ep->req);
> ep->ep = NULL;
> + mutex_unlock(&ep->lock);
> +
> wake_up (&ep->wait);
> put_ep (ep);
>
> - spin_unlock_irq (&dev->lock);
> -
> /* break link to dcache */
> inode_lock(parent);
> d_delete (dentry);
> @@ -1603,6 +1629,11 @@ gadgetfs_unbind (struct usb_gadget *gadg
>
> spin_lock_irq (&dev->lock);
> dev->state = STATE_DEV_UNBOUND;
> + while (dev->udc_usage > 0) {
> + spin_unlock_irq(&dev->lock);
> + usleep_range(1000, 2000);
> + spin_lock_irq(&dev->lock);
> + }
> spin_unlock_irq (&dev->lock);
>
> destroy_ep_files (dev);
>