usb: gadget: f_hid: fix: Move IN request allocation to set_alt()

Since commit: ba1582f22231 ("usb: gadget: f_hid: use alloc_ep_req()")
we cannot allocate any requests in bind() as we check if we should
align request buffer based on endpoint descriptor which is assigned
in set_alt().

Allocating request in bind() function causes a NULL pointer
dereference.

This commit moves allocation of IN request from bind() to set_alt()
to prevent this issue.

Fixes: ba1582f22231 ("usb: gadget: f_hid: use alloc_ep_req()")
Cc: stable@vger.kernel.org
Tested-by: David Lechner <david@lechnology.com>
Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index b62e69d..89b48bc 100644 (file)
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -338,6 +338,7 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
                            size_t count, loff_t *offp)
 {
        struct f_hidg *hidg  = file->private_data;
+       struct usb_request *req;
        unsigned long flags;
        ssize_t status = -ENOMEM;
 
@@ -347,7 +348,7 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
        spin_lock_irqsave(&hidg->write_spinlock, flags);
 
 #define WRITE_COND (!hidg->write_pending)
-
+try_again:
        /* write queue */
        while (!WRITE_COND) {
                spin_unlock_irqrestore(&hidg->write_spinlock, flags);
@@ -362,6 +363,7 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
        }
 
        hidg->write_pending = 1;
+       req = hidg->req;
        count  = min_t(unsigned, count, hidg->report_length);
 
        spin_unlock_irqrestore(&hidg->write_spinlock, flags);
@@ -374,24 +376,38 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
                goto release_write_pending;
        }
 
-       hidg->req->status   = 0;
-       hidg->req->zero     = 0;
-       hidg->req->length   = count;
-       hidg->req->complete = f_hidg_req_complete;
-       hidg->req->context  = hidg;
+       spin_lock_irqsave(&hidg->write_spinlock, flags);
+
+       /* we our function has been disabled by host */
+       if (!hidg->req) {
+               free_ep_req(hidg->in_ep, hidg->req);
+               /*
+                * TODO
+                * Should we fail with error here?
+                */
+               goto try_again;
+       }
+
+       req->status   = 0;
+       req->zero     = 0;
+       req->length   = count;
+       req->complete = f_hidg_req_complete;
+       req->context  = hidg;
 
        status = usb_ep_queue(hidg->in_ep, hidg->req, GFP_ATOMIC);
        if (status < 0) {
                ERROR(hidg->func.config->cdev,
                        "usb_ep_queue error on int endpoint %zd\n", status);
-               goto release_write_pending;
+               goto release_write_pending_unlocked;
        } else {
                status = count;
        }
+       spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
        return status;
 release_write_pending:
        spin_lock_irqsave(&hidg->write_spinlock, flags);
+release_write_pending_unlocked:
        hidg->write_pending = 0;
        spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
@@ -595,12 +611,23 @@ static void hidg_disable(struct usb_function *f)
                kfree(list);
        }
        spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+
+       spin_lock_irqsave(&hidg->write_spinlock, flags);
+       if (!hidg->write_pending) {
+               free_ep_req(hidg->in_ep, hidg->req);
+               hidg->write_pending = 1;
+       }
+
+       hidg->req = NULL;
+       spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 }
 
 static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 {
        struct usb_composite_dev                *cdev = f->config->cdev;
        struct f_hidg                           *hidg = func_to_hidg(f);
+       struct usb_request                      *req_in = NULL;
+       unsigned long                           flags;
        int i, status = 0;
 
        VDBG(cdev, "hidg_set_alt intf:%d alt:%d\n", intf, alt);
@@ -621,6 +648,12 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
                        goto fail;
                }
                hidg->in_ep->driver_data = hidg;
+
+               req_in = hidg_alloc_ep_req(hidg->in_ep, hidg->report_length);
+               if (!req_in) {
+                       status = -ENOMEM;
+                       goto disable_ep_in;
+               }
        }
 
 
@@ -632,12 +665,12 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
                                            hidg->out_ep);
                if (status) {
                        ERROR(cdev, "config_ep_by_speed FAILED!\n");
-                       goto fail;
+                       goto free_req_in;
                }
                status = usb_ep_enable(hidg->out_ep);
                if (status < 0) {
                        ERROR(cdev, "Enable OUT endpoint FAILED!\n");
-                       goto fail;
+                       goto free_req_in;
                }
                hidg->out_ep->driver_data = hidg;
 
@@ -653,17 +686,37 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
                                req->context  = hidg;
                                status = usb_ep_queue(hidg->out_ep, req,
                                                      GFP_ATOMIC);
-                               if (status)
+                               if (status) {
                                        ERROR(cdev, "%s queue req --> %d\n",
                                                hidg->out_ep->name, status);
+                                       free_ep_req(hidg->out_ep, req);
+                               }
                        } else {
-                               usb_ep_disable(hidg->out_ep);
                                status = -ENOMEM;
-                               goto fail;
+                               goto disable_out_ep;
                        }
                }
        }
 
+       if (hidg->in_ep != NULL) {
+               spin_lock_irqsave(&hidg->write_spinlock, flags);
+               hidg->req = req_in;
+               hidg->write_pending = 0;
+               spin_unlock_irqrestore(&hidg->write_spinlock, flags);
+
+               wake_up(&hidg->write_queue);
+       }
+       return 0;
+disable_out_ep:
+       usb_ep_disable(hidg->out_ep);
+free_req_in:
+       if (req_in)
+               free_ep_req(hidg->in_ep, req_in);
+
+disable_ep_in:
+       if (hidg->in_ep)
+               usb_ep_disable(hidg->in_ep);
+
 fail:
        return status;
 }
@@ -712,12 +765,6 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
                goto fail;
        hidg->out_ep = ep;
 
-       /* preallocate request and buffer */
-       status = -ENOMEM;
-       hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
-       if (!hidg->req)
-               goto fail;
-
        /* set descriptor dynamic values */
        hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
        hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
@@ -755,6 +802,8 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
                goto fail;
 
        spin_lock_init(&hidg->write_spinlock);
+       hidg->write_pending = 1;
+       hidg->req = NULL;
        spin_lock_init(&hidg->read_spinlock);
        init_waitqueue_head(&hidg->write_queue);
        init_waitqueue_head(&hidg->read_queue);
@@ -1019,10 +1068,6 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
        device_destroy(hidg_class, MKDEV(major, hidg->minor));
        cdev_del(&hidg->cdev);
 
-       /* disable/free request and end point */
-       usb_ep_disable(hidg->in_ep);
-       free_ep_req(hidg->in_ep, hidg->req);
-
        usb_free_all_descriptors(f);
 }