[PATCH 0/2] Two fix for dwc2 gadget driver

[PATCH 0/2] Two fix for dwc2 gadget driver

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

With the first patch, enable a enabled ep will return -EBUSY.
The second patch forbid queuing on disabled ep to avoid panic.

Du, Changbin (2):
  usb: dwc2: add ep enabled flag to avoid double enable/disable
  usb: dwc2: forbid queuing request to a disabled ep

 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 26 +++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

-- 
2.5.0

[PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Enabling a already enabled ep is illegal, because the ep may has trbs
running. Reprogram the ep may break running transfer. So udc driver
must avoid this happening by return an error -EBUSY. Gadget function
driver also should avoid such things, but that is out of udc driver.

Similarly, disable a disabled ep makes no sense, but no need return
an error here.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 20 +++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index a66d3cb..cf7eccd 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -162,6 +162,7 @@ struct dwc2_hsotg_ep {
 	unsigned char           mc;
 	unsigned char           interval;
 
+	unsigned int		enabled:1;
 	unsigned int            halted:1;
 	unsigned int            periodic:1;
 	unsigned int            isochronous:1;
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 0abf73c..586bbcd 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2423,6 +2423,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
 	/* enable, but don't activate EP0in */
 	dwc2_writel(dwc2_hsotg_ep0_mps(hsotg->eps_out[0]->ep.maxpacket) |
 	       DXEPCTL_USBACTEP, hsotg->regs + DIEPCTL0);
+	hsotg->eps_out[0]->enabled = 1;
 
 	dwc2_hsotg_enqueue_setup(hsotg);
 
@@ -2680,6 +2681,14 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
 		return -EINVAL;
 	}
 
+	spin_lock_irqsave(&hsotg->lock, flags);
+	if (hs_ep->enabled) {
+		dev_warn(hsotg->dev, "%s: ep %s already enabled\n",
+			__func__, hs_ep->name);
+		ret = -EBUSY;
+		goto error;
+	}
+
 	mps = usb_endpoint_maxp(desc);
 
 	/* note, we handle this here instead of dwc2_hsotg_set_ep_maxpacket */
@@ -2690,7 +2699,6 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
 	dev_dbg(hsotg->dev, "%s: read DxEPCTL=0x%08x from 0x%08x\n",
 		__func__, epctrl, epctrl_reg);
 
-	spin_lock_irqsave(&hsotg->lock, flags);
 
 	epctrl &= ~(DXEPCTL_EPTYPE_MASK | DXEPCTL_MPS_MASK);
 	epctrl |= DXEPCTL_MPS(mps);
@@ -2806,6 +2814,8 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
 	/* enable the endpoint interrupt */
 	dwc2_hsotg_ctrl_epint(hsotg, index, dir_in, 1);
 
+	hs_ep->enabled = 1;
+
 error:
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 	return ret;
@@ -2835,6 +2845,11 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
 	epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
 
 	spin_lock_irqsave(&hsotg->lock, flags);
+	if (!hs_ep->enabled) {
+		dev_warn(hsotg->dev, "%s: ep %s already disabled\n",
+			__func__, hs_ep->name);
+		goto out;
+	}
 
 	hsotg->fifo_map &= ~(1<<hs_ep->fifo_index);
 	hs_ep->fifo_index = 0;
@@ -2854,6 +2869,9 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
 	/* terminate all requests with shutdown */
 	kill_all_requests(hsotg, hs_ep, -ESHUTDOWN);
 
+	hs_ep->enabled = 0;
+
+out:
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 	return 0;
 }
-- 
2.5.0

[PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Queue a request to disabled ep  doesn't make sense, and induce caller
make mistakes.

Here is a example for the android mtp gadget function driver. A mem
corruption can happen on below senario.
1) On disconnect, mtp driver disable its EPs,
2) During send_file_work and receive_file_work, mtp queues a request
   to ep. (The mtp driver need improve its synchronization logic!)
3) mtp_function_unbind is invoked and all mtp requests are freed.
4) when dwc2 process the request queued on step 2, will cause kernel
   NULL pointer dereference exception.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 drivers/usb/dwc2/gadget.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 586bbcd..4d637ab 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -786,6 +786,12 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req,
 		ep->name, req, req->length, req->buf, req->no_interrupt,
 		req->zero, req->short_not_ok);
 
+	if (!hs_ep->enabled) {
+		dev_warn(hs->dev, "%s: cannot queue to disabled ep\n",
+				__func__);
+		return -ESHUTDOWN;
+	}
+
 	/* Prevent new request submission when controller is suspended */
 	if (hs->lx_state == DWC2_L2) {
 		dev_dbg(hs->dev, "%s: don't submit request while suspended\n",
-- 
2.5.0

Re: [PATCH 0/2] Two fix for dwc2 gadget driver

[John Youn]

On 11/29/2015 9:29 PM, changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@intel.com>
> 
> With the first patch, enable a enabled ep will return -EBUSY.
> The second patch forbid queuing on disabled ep to avoid panic.


The usb_ep->enabled flag was added in 4.4.

It looks like these same checks are also added at the API level in the
usb_ep_enable() and usb_ep_disable().

In case this is bypassed we should probably add them in the gadget
anyways but using the existing flag.

Regards,
John



> 
> Du, Changbin (2):
>   usb: dwc2: add ep enabled flag to avoid double enable/disable
>   usb: dwc2: forbid queuing request to a disabled ep
> 
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 26 +++++++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 

RE: [PATCH 0/2] Two fix for dwc2 gadget driver

[Du, Changbin]

> On 11/29/2015 9:29 PM, changbin.du@intel.com wrote:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > With the first patch, enable a enabled ep will return -EBUSY.
> > The second patch forbid queuing on disabled ep to avoid panic.
> 
> 
> The usb_ep->enabled flag was added in 4.4.
> 
> It looks like these same checks are also added at the API level in the
> usb_ep_enable() and usb_ep_disable().
> 
> In case this is bypassed we should probably add them in the gadget
> anyways but using the existing flag.
> 
> Regards,
> John
> 
Hmm, just learnt the flag on gadget API layer. And I just see usb_ep_enable return success if it is already enabled.
But I think it should return an error to inform the caller. Because the ep configuration may probably be changed.
In this case, usb_ep_enable will do different behavior.

Hmm, the usb_ep_queue doesn't check the enabled flag. Should be added. Let me have a try.

Best Regards,
Changbin

[PATCH] usb: gadget: forbid queuing request to a disabled ep

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Queue a request to disabled ep  doesn't make sense, and induce caller
make mistakes.

Here is a example for the android mtp gadget function driver. A mem
corruption can happen on below senario.
1) On disconnect, mtp driver disable its EPs,
2) During send_file_work and receive_file_work, mtp queues a request
   to ep. (The mtp driver need improve its synchronization logic!)
3) mtp_function_unbind is invoked and all mtp requests are freed.
4) when udc process the request queued on step 2, will cause kernel
   NULL pointer dereference exception.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
This patch is seprated from below patches because gadget layer has
added the 'enabled' flag in v4.4. so abandon it and submit new one.
[PATCH 0/2] Two fix for dwc2 gadget driver
  usb: dwc2: add ep enabled flag to avoid double enable/disable
  usb: dwc2: forbid queuing request to a disabled ep

---
 include/linux/usb/gadget.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3d583a1..d813bd2 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep,
 static inline int usb_ep_queue(struct usb_ep *ep,
 			       struct usb_request *req, gfp_t gfp_flags)
 {
+	if (!ep->enabled)
+		return -ESHUTDOWN;
+
 	return ep->ops->queue(ep, req, gfp_flags);
 }
 
-- 
2.5.0

Re: [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2108 bytes --]


Hi,

changbin.du@intel.com writes:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> Enabling a already enabled ep is illegal, because the ep may has trbs
> running. Reprogram the ep may break running transfer. So udc driver
> must avoid this happening by return an error -EBUSY. Gadget function
> driver also should avoid such things, but that is out of udc driver.
>
> Similarly, disable a disabled ep makes no sense, but no need return
> an error here.
>
> Signed-off-by: Du, Changbin <changbin.du@intel.com>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 20 +++++++++++++++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index a66d3cb..cf7eccd 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -162,6 +162,7 @@ struct dwc2_hsotg_ep {
>  	unsigned char           mc;
>  	unsigned char           interval;
>  
> +	unsigned int		enabled:1;
>  	unsigned int            halted:1;
>  	unsigned int            periodic:1;
>  	unsigned int            isochronous:1;
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 0abf73c..586bbcd 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2423,6 +2423,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>  	/* enable, but don't activate EP0in */
>  	dwc2_writel(dwc2_hsotg_ep0_mps(hsotg->eps_out[0]->ep.maxpacket) |
>  	       DXEPCTL_USBACTEP, hsotg->regs + DIEPCTL0);
> +	hsotg->eps_out[0]->enabled = 1;
>  
>  	dwc2_hsotg_enqueue_setup(hsotg);
>  
> @@ -2680,6 +2681,14 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
>  		return -EINVAL;
>  	}
>  
> +	spin_lock_irqsave(&hsotg->lock, flags);
> +	if (hs_ep->enabled) {
> +		dev_warn(hsotg->dev, "%s: ep %s already enabled\n",
> +			__func__, hs_ep->name);

this is a rather serious condition. I'd rather use dev_WARN_ONCE():

	if (dev_WARN_ONCE(hsotg->dev, hs_ep->enabled,
           	"ep %s already enabled\n", hs_ep->name)) {

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]


Hi,

changbin.du@intel.com writes:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> Queue a request to disabled ep  doesn't make sense, and induce caller
> make mistakes.
>
> Here is a example for the android mtp gadget function driver. A mem
> corruption can happen on below senario.
> 1) On disconnect, mtp driver disable its EPs,
> 2) During send_file_work and receive_file_work, mtp queues a request
>    to ep. (The mtp driver need improve its synchronization logic!)
> 3) mtp_function_unbind is invoked and all mtp requests are freed.
> 4) when dwc2 process the request queued on step 2, will cause kernel
>    NULL pointer dereference exception.
>
> Signed-off-by: Du, Changbin <changbin.du@intel.com>
> ---
>  drivers/usb/dwc2/gadget.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 586bbcd..4d637ab 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -786,6 +786,12 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req,
>  		ep->name, req, req->length, req->buf, req->no_interrupt,
>  		req->zero, req->short_not_ok);
>  
> +	if (!hs_ep->enabled) {
> +		dev_warn(hs->dev, "%s: cannot queue to disabled ep\n",
> +				__func__);

similar comment to previous patch:

	if (dev_WARN_ONCE(hs->dev, !hs_ep->enabled,
        	"cannot queue to disabled ep %s\n", hs_ep->name))

> +		return -ESHUTDOWN;
> +	}
> +
>  	/* Prevent new request submission when controller is suspended */
>  	if (hs->lx_state == DWC2_L2) {
>  		dev_dbg(hs->dev, "%s: don't submit request while suspended\n",
> -- 
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: gadget: forbid queuing request to a disabled ep

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]


Hi,

changbin.du@intel.com writes:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> Queue a request to disabled ep  doesn't make sense, and induce caller
> make mistakes.
>
> Here is a example for the android mtp gadget function driver. A mem
> corruption can happen on below senario.
> 1) On disconnect, mtp driver disable its EPs,
> 2) During send_file_work and receive_file_work, mtp queues a request
>    to ep. (The mtp driver need improve its synchronization logic!)
> 3) mtp_function_unbind is invoked and all mtp requests are freed.
> 4) when udc process the request queued on step 2, will cause kernel
>    NULL pointer dereference exception.
>
> Signed-off-by: Du, Changbin <changbin.du@intel.com>
> ---
> This patch is seprated from below patches because gadget layer has
> added the 'enabled' flag in v4.4. so abandon it and submit new one.
> [PATCH 0/2] Two fix for dwc2 gadget driver
>   usb: dwc2: add ep enabled flag to avoid double enable/disable
>   usb: dwc2: forbid queuing request to a disabled ep
>
> ---
>  include/linux/usb/gadget.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3d583a1..d813bd2 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep,
>  static inline int usb_ep_queue(struct usb_ep *ep,
>  			       struct usb_request *req, gfp_t gfp_flags)
>  {
> +	if (!ep->enabled)
> +		return -ESHUTDOWN;

same warn here:

	if (WARN_ON_ONCE(!ep->enabled))
        	return -ESHUTDOWN;

> +
>  	return ep->ops->queue(ep, req, gfp_flags);
>  }
>  
> -- 
> 2.5.0
>

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

RE: [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable

[Du, Changbin]

Hi, Balbi,
Please ignore this patch set. Because we have a fix in gadget API layer.
[PATCH] usb: gadget: forbid queuing request to a disabled ep

> > Enabling an already enabled ep is illegal, because the ep may has trbs
> > running. Reprogram the ep may break running transfer. So udc driver
> > must avoid this happening by return an error -EBUSY. Gadget function
> > driver also should avoid such things, but that is out of udc driver.
> >
> > Similarly, disable a disabled ep makes no sense, but no need return
> > an error here.
> >
> > Signed-off-by: Du, Changbin <changbin.du@intel.com>
> > ---
> >  drivers/usb/dwc2/core.h   |  1 +
> >  drivers/usb/dwc2/gadget.c | 20 +++++++++++++++++++-
> >  2 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > index a66d3cb..cf7eccd 100644
> > --- a/drivers/usb/dwc2/core.h
> > +++ b/drivers/usb/dwc2/core.h
> > @@ -162,6 +162,7 @@ struct dwc2_hsotg_ep {
> >  	unsigned char           mc;
> >  	unsigned char           interval;
> >
> > +	unsigned int		enabled:1;
> >  	unsigned int            halted:1;
> >  	unsigned int            periodic:1;
> >  	unsigned int            isochronous:1;
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 0abf73c..586bbcd 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -2423,6 +2423,7 @@ void dwc2_hsotg_core_init_disconnected(struct
> dwc2_hsotg *hsotg,
> >  	/* enable, but don't activate EP0in */
> >  	dwc2_writel(dwc2_hsotg_ep0_mps(hsotg->eps_out[0]-
> >ep.maxpacket) |
> >  	       DXEPCTL_USBACTEP, hsotg->regs + DIEPCTL0);
> > +	hsotg->eps_out[0]->enabled = 1;
> >
> >  	dwc2_hsotg_enqueue_setup(hsotg);
> >
> > @@ -2680,6 +2681,14 @@ static int dwc2_hsotg_ep_enable(struct usb_ep
> *ep,
> >  		return -EINVAL;
> >  	}
> >
> > +	spin_lock_irqsave(&hsotg->lock, flags);
> > +	if (hs_ep->enabled) {
> > +		dev_warn(hsotg->dev, "%s: ep %s already enabled\n",
> > +			__func__, hs_ep->name);
> 
> this is a rather serious condition. I'd rather use dev_WARN_ONCE():
> 
> 	if (dev_WARN_ONCE(hsotg->dev, hs_ep->enabled,
>            	"ep %s already enabled\n", hs_ep->name)) {
> 
> --
> balbi

[PATCH v2] usb: gadget: forbid queuing request to a disabled ep

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Queue a request to disabled ep  doesn't make sense, and induce caller
make mistakes.

Here is a example for the android mtp gadget function driver. A mem
corruption can happen on below senario.
1) On disconnect, mtp driver disable its EPs,
2) During send_file_work and receive_file_work, mtp queues a request
   to ep. (The mtp driver need improve its synchronization logic!)
3) mtp_function_unbind is invoked and all mtp requests are freed.
4) when udc process the request queued on step 2, will cause kernel
   NULL pointer dereference exception.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
change from v1: add WARN_ON_ONCE message.

---
 include/linux/usb/gadget.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3d583a1..b566a4b 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep,
 static inline int usb_ep_queue(struct usb_ep *ep,
 			       struct usb_request *req, gfp_t gfp_flags)
 {
+	if (WARN_ON_ONCE(!ep->enabled))
+		return -ESHUTDOWN;
+
 	return ep->ops->queue(ep, req, gfp_flags);
 }
 
-- 
2.5.0

RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep

[Du, Changbin]

> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3d583a1..b566a4b 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep
> *ep,
>  static inline int usb_ep_queue(struct usb_ep *ep,
>  			       struct usb_request *req, gfp_t gfp_flags)
>  {
> +	if (WARN_ON_ONCE(!ep->enabled))
> +		return -ESHUTDOWN;
> +
>  	return ep->ops->queue(ep, req, gfp_flags);
>  }
> 
> --
> 2.5.0
With  this patch, ep0 transfer breaks. it because the 'enabled' of ep0  is not set. Ep0 
is not enabled by usb_ep_enable, but in UDC driver. So there need another patch
to set ep0's flag also.

RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]


Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 3d583a1..b566a4b 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep
>> *ep,
>>  static inline int usb_ep_queue(struct usb_ep *ep,
>>  			       struct usb_request *req, gfp_t gfp_flags)
>>  {
>> +	if (WARN_ON_ONCE(!ep->enabled))
>> +		return -ESHUTDOWN;
>> +
>>  	return ep->ops->queue(ep, req, gfp_flags);
>>  }
>> 
>> --
>> 2.5.0
>
> With this patch, ep0 transfer breaks. it because the 'enabled' of ep0
> is not set. Ep0 is not enabled by usb_ep_enable, but in UDC driver. So
> there need another patch to set ep0's flag also.

yeah, we don't like regressions :-) So the fix should come before
$subject to avoid a regression.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep

[Du, Changbin]

> >> 2.5.0
> >
> > With this patch, ep0 transfer breaks. it because the 'enabled' of ep0
> > is not set. Ep0 is not enabled by usb_ep_enable, but in UDC driver. So
> > there need another patch to set ep0's flag also.
> 
> yeah, we don't like regressions :-) So the fix should come before
> $subject to avoid a regression.
> 
> --
> balbi
It is hard to determine if ep0 is enabled or not in gadget API layer. Because
it is controlled by udc driver, it may enable it at pullup, vbussession...
But here, we can ignore for control-ep, considering it always enabled during
usb session.

[PATCH v3] usb: gadget: forbid queuing request to a disabled ep

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Queue a request to disabled ep  doesn't make sense, and induce caller
make mistakes.

Here is a example for the android mtp gadget function driver. A mem
corruption can happen on below senario.
1) On disconnect, mtp driver disable its EPs,
2) During send_file_work and receive_file_work, mtp queues a request
   to ep. (The mtp driver need improve its synchronization logic!)
3) mtp_function_unbind is invoked and all mtp requests are freed.
4) when udc process the request queued on step 2, will cause kernel
   NULL pointer dereference exception.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
change from v2: igonre ep0 as it always enabled during usb session.
change from v1: add WARN_ON_ONCE message.
---
 include/linux/usb/gadget.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3d583a1..0c5d9ea 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep,
 static inline int usb_ep_queue(struct usb_ep *ep,
 			       struct usb_request *req, gfp_t gfp_flags)
 {
+	if (WARN_ON_ONCE(!ep->enabled && !ep->address))
+		return -ESHUTDOWN;
+
 	return ep->ops->queue(ep, req, gfp_flags);
 }
 
-- 
2.5.0

Re: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]


Hi,

changbin.du@intel.com writes:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> Queue a request to disabled ep  doesn't make sense, and induce caller
> make mistakes.
>
> Here is a example for the android mtp gadget function driver. A mem
> corruption can happen on below senario.
> 1) On disconnect, mtp driver disable its EPs,
> 2) During send_file_work and receive_file_work, mtp queues a request
>    to ep. (The mtp driver need improve its synchronization logic!)
> 3) mtp_function_unbind is invoked and all mtp requests are freed.
> 4) when udc process the request queued on step 2, will cause kernel
>    NULL pointer dereference exception.
>
> Signed-off-by: Du, Changbin <changbin.du@intel.com>
> ---
> change from v2: igonre ep0 as it always enabled during usb session.
> change from v1: add WARN_ON_ONCE message.
> ---
>  include/linux/usb/gadget.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3d583a1..0c5d9ea 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep,
>  static inline int usb_ep_queue(struct usb_ep *ep,
>  			       struct usb_request *req, gfp_t gfp_flags)
>  {
> +	if (WARN_ON_ONCE(!ep->enabled && !ep->address))

this will only trigger for a disabled ep0. Are you testing any of your
patches at all ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

RE: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep

[Du, Changbin]

> >
> > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> > index 3d583a1..0c5d9ea 100644
> > --- a/include/linux/usb/gadget.h
> > +++ b/include/linux/usb/gadget.h
> > @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct
> usb_ep *ep,
> >  static inline int usb_ep_queue(struct usb_ep *ep,
> >  			       struct usb_request *req, gfp_t gfp_flags)
> >  {
> > +	if (WARN_ON_ONCE(!ep->enabled && !ep->address))
> 
> this will only trigger for a disabled ep0. Are you testing any of your
> patches at all ?
> 
> --
> balbi
Oops, I sent a wrong patch. I will send right patch again as v4, very sorry for this.
The right patch has been verified on 3.14 by back-porting related 1 patch.

[PATCH v4] usb: gadget: forbid queuing request to a disabled ep

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Queue a request to disabled ep  doesn't make sense, and induce caller
make mistakes.

Here is a example for the android mtp gadget function driver. A mem
corruption can happen on below senario.
1) On disconnect, mtp driver disable its EPs,
2) During send_file_work and receive_file_work, mtp queues a request
   to ep. (The mtp driver need improve its synchronization logic!)
3) mtp_function_unbind is invoked and all mtp requests are freed.
4) when udc process the request queued on step 2, will cause kernel
   NULL pointer dereference exception.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
change from v3: fix v3's error.
change from v2: igonre ep0 as it always enabled during usb session.
change from v1: add WARN_ON_ONCE message.

---
 include/linux/usb/gadget.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3d583a1..fc78687 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep,
 static inline int usb_ep_queue(struct usb_ep *ep,
 			       struct usb_request *req, gfp_t gfp_flags)
 {
+	if (WARN_ON_ONCE(!ep->enabled && ep->address))
+		return -ESHUTDOWN;
+
 	return ep->ops->queue(ep, req, gfp_flags);
 }
 
-- 
2.5.0