[v2] usb: raw-gadget: fix raw_event_queue_fetch locking

Commit Message

Andrey Konovalov April 7, 2020, 2:14 p.m. UTC

If queue->size check in raw_event_queue_fetch() fails (which normally
shouldn't happen, that check is a fail-safe), the function returns
without reenabling interrupts. This patch fixes that issue, along with
propagating the cause of failure to the function caller.

Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface"
Reported-by: Dan Carpenter <[e-mail]>
Signed-off-by: Andrey Konovalov <[e-mail]>
---

Changes in v2:
- Added a comment before the WARN_ON() call.

Greg, this should apply cleanly on top of Dan's "usb: raw-gadget: Fix
copy_to/from_user() checks" patch.

---
 drivers/usb/gadget/legacy/raw_gadget.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

Dan Carpenter April 7, 2020, 2:29 p.m. UTC | #1

On Tue, Apr 07, 2020 at 04:14:50PM +0200, Andrey Konovalov wrote:
> @@ -89,11 +90,18 @@ static struct usb_raw_event *raw_event_queue_fetch(
>  	 * there's at least one event queued by decrementing the semaphore,
>  	 * and then take the lock to protect queue struct fields.
>  	 */
> -	if (down_interruptible(&queue->sema))
> -		return NULL;
> +	ret = down_interruptible(&queue->sema);
> +	if (ret)
> +		return ERR_PTR(ret);
>  	spin_lock_irqsave(&queue->lock, flags);
> -	if (WARN_ON(!queue->size))
> +	/*
> +	 * queue->size must have the same value as queue->sema counter (before
> +	 * the down_interruptible() call above), so this check is a fail-safe.
> +	 */
> +	if (WARN_ON(!queue->size)) {
> +		spin_unlock_irqrestore(&queue->lock, flags);
>  		return NULL;

I'm sorry for not noticing this earlier.  When a function returns both
error pointers and NULL then NULL is supposed to a special case of
success.  For example:

	my_struct_pointer = get_optional_feature();

If there is a memory allocation failure then my_struct_pointer is
-ENOMEM and we fail.  But say the optional feature is disabled, then
we can't return a valid pointer, but it's also working as designed so
it's not an error.  In that case we return NULL.  The surrounding code
should be written to allow NULL pointers.

So I don't think returning NULL here is correct.

regards,
dan carpenter

Andrey Konovalov April 7, 2020, 2:48 p.m. UTC | #2

On Tue, Apr 7, 2020 at 4:29 PM Dan Carpenter <[e-mail]> wrote:
>
> On Tue, Apr 07, 2020 at 04:14:50PM +0200, Andrey Konovalov wrote:
> > @@ -89,11 +90,18 @@ static struct usb_raw_event *raw_event_queue_fetch(
> >        * there's at least one event queued by decrementing the semaphore,
> >        * and then take the lock to protect queue struct fields.
> >        */
> > -     if (down_interruptible(&queue->sema))
> > -             return NULL;
> > +     ret = down_interruptible(&queue->sema);
> > +     if (ret)
> > +             return ERR_PTR(ret);
> >       spin_lock_irqsave(&queue->lock, flags);
> > -     if (WARN_ON(!queue->size))
> > +     /*
> > +      * queue->size must have the same value as queue->sema counter (before
> > +      * the down_interruptible() call above), so this check is a fail-safe.
> > +      */
> > +     if (WARN_ON(!queue->size)) {
> > +             spin_unlock_irqrestore(&queue->lock, flags);
> >               return NULL;
>
> I'm sorry for not noticing this earlier.  When a function returns both
> error pointers and NULL then NULL is supposed to a special case of
> success.  For example:
>
>         my_struct_pointer = get_optional_feature();
>
> If there is a memory allocation failure then my_struct_pointer is
> -ENOMEM and we fail.  But say the optional feature is disabled, then
> we can't return a valid pointer, but it's also working as designed so
> it's not an error.  In that case we return NULL.  The surrounding code
> should be written to allow NULL pointers.
>
> So I don't think returning NULL here is correct.

No problem, sent v3, thanks!

Patch

diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
index e490ffa1f58b..85dfbcd461ac 100644
--- a/drivers/usb/gadget/legacy/raw_gadget.c
+++ b/drivers/usb/gadget/legacy/raw_gadget.c
@@ -81,6 +81,7 @@  static int raw_event_queue_add(struct raw_event_queue *queue,
 static struct usb_raw_event *raw_event_queue_fetch(
 				struct raw_event_queue *queue)
 {
+	int ret;
 	unsigned long flags;
 	struct usb_raw_event *event;
 
@@ -89,11 +90,18 @@  static struct usb_raw_event *raw_event_queue_fetch(
 	 * there's at least one event queued by decrementing the semaphore,
 	 * and then take the lock to protect queue struct fields.
 	 */
-	if (down_interruptible(&queue->sema))
-		return NULL;
+	ret = down_interruptible(&queue->sema);
+	if (ret)
+		return ERR_PTR(ret);
 	spin_lock_irqsave(&queue->lock, flags);
-	if (WARN_ON(!queue->size))
+	/*
+	 * queue->size must have the same value as queue->sema counter (before
+	 * the down_interruptible() call above), so this check is a fail-safe.
+	 */
+	if (WARN_ON(!queue->size)) {
+		spin_unlock_irqrestore(&queue->lock, flags);
 		return NULL;
+	}
 	event = queue->events[0];
 	queue->size--;
 	memmove(&queue->events[0], &queue->events[1],
@@ -522,10 +530,17 @@  static int raw_ioctl_event_fetch(struct raw_dev *dev, unsigned long value)
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	event = raw_event_queue_fetch(&dev->queue);
-	if (!event) {
+	if (PTR_ERR(event) == -EINTR) {
 		dev_dbg(&dev->gadget->dev, "event fetching interrupted\n");
 		return -EINTR;
 	}
+	if (IS_ERR_OR_NULL(event)) {
+		dev_err(&dev->gadget->dev, "failed to fetch event\n");
+		spin_lock_irqsave(&dev->lock, flags);
+		dev->state = STATE_DEV_FAILED;
+		spin_unlock_irqrestore(&dev->lock, flags);
+		return -ENODEV;
+	}
 	length = min(arg.length, event->length);
 	if (copy_to_user((void __user *)value, event, sizeof(*event) + length))
 		return -EFAULT;