[PATCH] usb: gadget: gadgetfs: correct dev state

[PATCH] usb: gadget: gadgetfs: correct dev state

[Marcus Nutzinger]

Commit 1826e9b1 fixes the use after free of "dev".
However if this is not the final call to dev_release()
and the state is not reset to STATE_DEV_DISABLED and
hence all further open() calls to the gadgetfs ep0
device will fail with EBUSY.

So this commit reverts 1826e9b1 and places the call
put_dev() after setting the state.

Signed-off-by: Marcus Nutzinger <marcus.nutzinger@theobroma-systems.com>
Reviewed-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---
 drivers/usb/gadget/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index a925d0c..6330528 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -1264,8 +1264,13 @@ dev_release (struct inode *inode, struct file *fd)
 
 	kfree (dev->buf);
 	dev->buf = NULL;
-	put_dev (dev);
 
+	/* other endpoints were all decoupled from this device */
+	spin_lock_irq(&dev->lock);
+	dev->state = STATE_DEV_DISABLED;
+	spin_unlock_irq(&dev->lock);
+
+	put_dev (dev);
 	return 0;
 }
 
-- 
1.9.0

Re: [PATCH] usb: gadget: gadgetfs: correct dev state

[Sergei Shtylyov]

Hello.

On 06/05/2014 05:08 PM, Marcus Nutzinger wrote:

> Commit 1826e9b1 fixes the use after free of "dev".

    Please also specify that commit's summary line in parens.

> However if this is not the final call to dev_release()
> and the state is not reset to STATE_DEV_DISABLED and
> hence all further open() calls to the gadgetfs ep0
> device will fail with EBUSY.

> So this commit reverts 1826e9b1 and places the call
> put_dev() after setting the state.

> Signed-off-by: Marcus Nutzinger <marcus.nutzinger@theobroma-systems.com>
> Reviewed-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> ---
>   drivers/usb/gadget/inode.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)

> diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
> index a925d0c..6330528 100644
> --- a/drivers/usb/gadget/inode.c
> +++ b/drivers/usb/gadget/inode.c
> @@ -1264,8 +1264,13 @@ dev_release (struct inode *inode, struct file *fd)
>
>   	kfree (dev->buf);
>   	dev->buf = NULL;
> -	put_dev (dev);
>
> +	/* other endpoints were all decoupled from this device */
> +	spin_lock_irq(&dev->lock);
> +	dev->state = STATE_DEV_DISABLED;
> +	spin_unlock_irq(&dev->lock);

    Not sure I understand why you need spinlock here... isn't the assignment 
atomic already?

> +
> +	put_dev (dev);
>   	return 0;
>   }

WBR, Sergei

Re: [PATCH] usb: gadget: gadgetfs: correct dev state

[Marcus Nutzinger]

Hi Sergei,

On Jun 5, 2014, at 4:18 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:

>   Please also specify that commit's summary line in parens.

I'll resubmit the updated patch in a minute!

>> +	/* other endpoints were all decoupled from this device */
>> +	spin_lock_irq(&dev->lock);
>> +	dev->state = STATE_DEV_DISABLED;
>> +	spin_unlock_irq(&dev->lock);
> 
>   Not sure I understand why you need spinlock here... isn't the assignment atomic already?


Sure, an assignment might be atomic. However, following the policy of commit 7489d149
(USB: gadgetfs cleanups) all ep0 state changes shall be protected by spinlocks.

Thanks,
Marcus

Re: [PATCH] usb: gadget: gadgetfs: correct dev state

[Alan Stern]

On Thu, 5 Jun 2014, Marcus Nutzinger wrote:

> Hi Sergei,
> 
> On Jun 5, 2014, at 4:18 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:
> 
> >   Please also specify that commit's summary line in parens.
> 
> I'll resubmit the updated patch in a minute!
> 
> >> +	/* other endpoints were all decoupled from this device */
> >> +	spin_lock_irq(&dev->lock);
> >> +	dev->state = STATE_DEV_DISABLED;
> >> +	spin_unlock_irq(&dev->lock);
> > 
> >   Not sure I understand why you need spinlock here... isn't the assignment atomic already?
> 
> 
> Sure, an assignment might be atomic. However, following the policy of commit 7489d149
> (USB: gadgetfs cleanups) all ep0 state changes shall be protected by spinlocks.

Sometimes an assignment needs to be protected by a lock, even though 
the assignment itself is atomic.  This happens, for example, when some 
other code executes a lock-protected region that expects the variable 
not to change.

I don't know if that's the case here.  But this example shows that in
general, one sometimes needs locks in places where you wouldn't expect
them.

In fact, it may even be necessary to take and release a lock, without
doing anything in between!

Alan Stern

[PATCH v2] usb: gadget: gadgetfs: correct dev state

[Marcus Nutzinger]

This reverts commit 1826e9b1 (usb: gadget: gadgetfs: use
after free in dev_release()) and places the call to
put_dev() after setting the state.

If this is not the final call to dev_release() and the
state is not reset to STATE_DEV_DISABLED and hence all
further open() calls to the gadgetfs ep0 device will
fail with EBUSY.

Signed-off-by: Marcus Nutzinger <marcus.nutzinger@theobroma-systems.com>
Reviewed-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---
 drivers/usb/gadget/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index a925d0c..6330528 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -1264,8 +1264,13 @@ dev_release (struct inode *inode, struct file *fd)
 
 	kfree (dev->buf);
 	dev->buf = NULL;
-	put_dev (dev);
 
+	/* other endpoints were all decoupled from this device */
+	spin_lock_irq(&dev->lock);
+	dev->state = STATE_DEV_DISABLED;
+	spin_unlock_irq(&dev->lock);
+
+	put_dev (dev);
 	return 0;
 }
 
-- 
1.9.0