[PATCH] usb: gadget: fsl_udc_core: do not immediatly prime STATUS for IN xfer

Because the fsl_udc_core driver shares one 'status_req' object for the
complete ep0 control transfer, it is not possible to prime the final
STATUS phase immediately after the IN transaction.  E.g. ch9getstatus()
executed:

| req = udc->status_req;
| ...
| list_add_tail(&req->queue, &ep->queue);
| if (ep0_prime_status(udc, EP_DIR_OUT))
|       ....
|       struct fsl_req *req = udc->status_req;
|       list_add_tail(&req->queue, &ep->queue);

which corrupts the ep->queue list by inserting 'status_req' twice.  This
causes a kernel oops e.g. when 'lsusb -v' is executed on the host.

Patch delays the final 'ep0_prime_status(udc, EP_DIR_OUT))' by moving it
into the ep0 completion handler.

Signed-off-by: Enrico Scholz <redacted>
---
 drivers/usb/gadget/fsl_udc_core.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index d7138cc..55c4a61 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c

@@ -1294,8 +1294,7 @@ static int ep0_prime_status(struct fsl_udc *udc, int direction)
 		udc->ep0_dir = USB_DIR_OUT;
 
 	ep = &udc->eps[0];
-	if (udc->ep0_state != DATA_STATE_XMIT)
-		udc->ep0_state = WAIT_FOR_OUT_STATUS;
+	udc->ep0_state = WAIT_FOR_OUT_STATUS;
 
 	req->ep = ep;
 	req->req.length = 0;

@@ -1400,8 +1399,6 @@ static void ch9getstatus(struct fsl_udc *udc, u8 request_type, u16 value,
 
 	list_add_tail(&req->queue, &ep->queue);
 	udc->ep0_state = DATA_STATE_XMIT;
-	if (ep0_prime_status(udc, EP_DIR_OUT))
-		ep0stall(udc);
 
 	return;
 stall:

@@ -1511,14 +1508,6 @@ static void setup_received_irq(struct fsl_udc *udc,
 		spin_lock(&udc->lock);
 		udc->ep0_state = (setup->bRequestType & USB_DIR_IN)
 				?  DATA_STATE_XMIT : DATA_STATE_RECV;
-		/*
-		 * If the data stage is IN, send status prime immediately.
-		 * See 2.0 Spec chapter 8.5.3.3 for detail.
-		 */
-		if (udc->ep0_state == DATA_STATE_XMIT)
-			if (ep0_prime_status(udc, EP_DIR_OUT))
-				ep0stall(udc);
-
 	} else {
 		/* No data phase, IN status from gadget */
 		udc->ep0_dir = USB_DIR_IN;

@@ -1548,7 +1537,8 @@ static void ep0_req_complete(struct fsl_udc *udc, struct fsl_ep *ep0,
 	switch (udc->ep0_state) {
 	case DATA_STATE_XMIT:
 		/* already primed at setup_received_irq */
-		udc->ep0_state = WAIT_FOR_OUT_STATUS;
+		if (ep0_prime_status(udc, EP_DIR_OUT))
+			ep0stall(udc);
 		break;
 	case DATA_STATE_RECV:
 		/* send status phase */

-- 
1.7.11.4

=20

=20
Because the fsl_udc_core driver shares one 'status_req' object for the
complete ep0 control transfer, it is not possible to prime the final
STATUS phase immediately after the IN transaction.  E.g. ch9getstatus()
executed:
=20
| req =3D udc->status_req;
| ...
| list_add_tail(&req->queue, &ep->queue);
| if (ep0_prime_status(udc, EP_DIR_OUT))
|       ....
|       struct fsl_req *req =3D udc->status_req;
|       list_add_tail(&req->queue, &ep->queue);
=20
which corrupts the ep->queue list by inserting 'status_req' twice.  This
causes a kernel oops e.g. when 'lsusb -v' is executed on the host.
=20
Patch delays the final 'ep0_prime_status(udc, EP_DIR_OUT))' by moving it
into the ep0 completion handler.
=20

Enrico, thanks for pointing this problem.

As "prime STATUS phase immediately after the IN transaction" is followed
USB 2.0 spec, to fix this problem, it is better to add data_req for ep0.
In fact, it is already at FSL i.mx internal code, just still not mainlined.

Peter
=20

On Wed, Sep 05, 2012 at 02:10:39AM +0000, Chen Peter-B29397 wrote:

quoted

Because the fsl_udc_core driver shares one 'status_req' object for the
complete ep0 control transfer, it is not possible to prime the final
STATUS phase immediately after the IN transaction.  E.g. ch9getstatus()
executed:

| req = udc->status_req;
| ...
| list_add_tail(&req->queue, &ep->queue);
| if (ep0_prime_status(udc, EP_DIR_OUT))
|       ....
|       struct fsl_req *req = udc->status_req;
|       list_add_tail(&req->queue, &ep->queue);

which corrupts the ep->queue list by inserting 'status_req' twice.  This
causes a kernel oops e.g. when 'lsusb -v' is executed on the host.

Patch delays the final 'ep0_prime_status(udc, EP_DIR_OUT))' by moving it
into the ep0 completion handler.

Enrico, thanks for pointing this problem.

As "prime STATUS phase immediately after the IN transaction" is followed
USB 2.0 spec, to fix this problem, it is better to add data_req for ep0.
In fact, it is already at FSL i.mx internal code, just still not mainlined.

so, do I get an Acked-by to this patch ? Does it need to go on v3.6-rc
or can it wait until v3.7 merge window ?

-- 
balbi

Felipe Balbi [off-list ref] writes:

quoted

quoted

Because the fsl_udc_core driver shares one 'status_req' object for the
complete ep0 control transfer, it is not possible to prime the final
STATUS phase immediately after the IN transaction.  E.g. ch9getstatus()
executed:

| req = udc->status_req;
| ...
| list_add_tail(&req->queue, &ep->queue);
| if (ep0_prime_status(udc, EP_DIR_OUT))
|       ....
|       struct fsl_req *req = udc->status_req;
|       list_add_tail(&req->queue, &ep->queue);

which corrupts the ep->queue list by inserting 'status_req' twice.  This
causes a kernel oops e.g. when 'lsusb -v' is executed on the host.

Patch delays the final 'ep0_prime_status(udc, EP_DIR_OUT))' by moving it
into the ep0 completion handler.

Enrico, thanks for pointing this problem.

As "prime STATUS phase immediately after the IN transaction" is followed
USB 2.0 spec, to fix this problem, it is better to add data_req for ep0.
In fact, it is already at FSL i.mx internal code, just still not mainlined.

so, do I get an Acked-by to this patch ? Does it need to go on v3.6-rc
or can it wait until v3.7 merge window ?

Without this (or the mentioned data_req patch), I can crash a g_multi
gadget by executing 'lsusb -v' as root on the host.  Should not be
exploitable (only a BUG_ON() is triggered) but issue should be fixed
asap.


Enrico

Hi,

On Thu, Sep 06, 2012 at 04:27:12PM +0200, Enrico Scholz wrote:

Felipe Balbi [off-list ref] writes:

quoted

quoted

quoted

Because the fsl_udc_core driver shares one 'status_req' object for the
complete ep0 control transfer, it is not possible to prime the final
STATUS phase immediately after the IN transaction.  E.g. ch9getstatus()
executed:

| req = udc->status_req;
| ...
| list_add_tail(&req->queue, &ep->queue);
| if (ep0_prime_status(udc, EP_DIR_OUT))
|       ....
|       struct fsl_req *req = udc->status_req;
|       list_add_tail(&req->queue, &ep->queue);

which corrupts the ep->queue list by inserting 'status_req' twice.  This
causes a kernel oops e.g. when 'lsusb -v' is executed on the host.

Patch delays the final 'ep0_prime_status(udc, EP_DIR_OUT))' by moving it
into the ep0 completion handler.

Enrico, thanks for pointing this problem.

As "prime STATUS phase immediately after the IN transaction" is followed
USB 2.0 spec, to fix this problem, it is better to add data_req for ep0.
In fact, it is already at FSL i.mx internal code, just still not mainlined.

so, do I get an Acked-by to this patch ? Does it need to go on v3.6-rc
or can it wait until v3.7 merge window ?

Without this (or the mentioned data_req patch), I can crash a g_multi
gadget by executing 'lsusb -v' as root on the host.  Should not be
exploitable (only a BUG_ON() is triggered) but issue should be fixed
asap.

cool, so I'll apply to my fixes branch as soon as I get Acked-by or
Tested-by from someone.

cheers

-- 
balbi

On Thu, Sep 06, 2012 at 05:27:42PM +0300, Felipe Balbi wrote:

Hi,

On Thu, Sep 06, 2012 at 04:27:12PM +0200, Enrico Scholz wrote:

quoted

Felipe Balbi [off-list ref] writes:

quoted

quoted

quoted

Because the fsl_udc_core driver shares one 'status_req' object for the
complete ep0 control transfer, it is not possible to prime the final
STATUS phase immediately after the IN transaction.  E.g. ch9getstatus()
executed:

| req = udc->status_req;
| ...
| list_add_tail(&req->queue, &ep->queue);
| if (ep0_prime_status(udc, EP_DIR_OUT))
|       ....
|       struct fsl_req *req = udc->status_req;
|       list_add_tail(&req->queue, &ep->queue);

which corrupts the ep->queue list by inserting 'status_req' twice.  This
causes a kernel oops e.g. when 'lsusb -v' is executed on the host.

Patch delays the final 'ep0_prime_status(udc, EP_DIR_OUT))' by moving it
into the ep0 completion handler.

Enrico, thanks for pointing this problem.

As "prime STATUS phase immediately after the IN transaction" is followed
USB 2.0 spec, to fix this problem, it is better to add data_req for ep0.
In fact, it is already at FSL i.mx internal code, just still not mainlined.

so, do I get an Acked-by to this patch ? Does it need to go on v3.6-rc
or can it wait until v3.7 merge window ?

Without this (or the mentioned data_req patch), I can crash a g_multi
gadget by executing 'lsusb -v' as root on the host.  Should not be
exploitable (only a BUG_ON() is triggered) but issue should be fixed
asap.

cool, so I'll apply to my fixes branch as soon as I get Acked-by or
Tested-by from someone.

No Acks, no Tested-by ?

-- 
balbi

-----Original Message-----
From: Felipe Balbi [mailto:balbi@ti.com]
Sent: Thursday, September 06, 2012 10:28 PM
To: Enrico Scholz
Cc: balbi@ti.com; Chen Peter-B29397; linux-usb@vger.kernel.org; linuxppc-
dev@lists.ozlabs.org; Li Yang-R58472; gregkh@linuxfoundation.org
Subject: Re: [PATCH] usb: gadget: fsl_udc_core: do not immediatly prime
STATUS for IN xfer
=20
Hi,
=20
On Thu, Sep 06, 2012 at 04:27:12PM +0200, Enrico Scholz wrote:

quoted

Felipe Balbi [off-list ref] writes:

quoted

quoted

quoted

Because the fsl_udc_core driver shares one 'status_req' object
for the complete ep0 control transfer, it is not possible to
prime the final STATUS phase immediately after the IN
transaction.  E.g. ch9getstatus()
executed:

| req =3D udc->status_req;
| ...
| list_add_tail(&req->queue, &ep->queue); if
| (ep0_prime_status(udc, EP_DIR_OUT))
|       ....
|       struct fsl_req *req =3D udc->status_req;
|       list_add_tail(&req->queue, &ep->queue);

which corrupts the ep->queue list by inserting 'status_req'
twice.  This causes a kernel oops e.g. when 'lsusb -v' is executed

on the host.

quoted

quoted

quoted

quoted

Patch delays the final 'ep0_prime_status(udc, EP_DIR_OUT))' by
moving it into the ep0 completion handler.

Enrico, thanks for pointing this problem.

As "prime STATUS phase immediately after the IN transaction" is
followed USB 2.0 spec, to fix this problem, it is better to add

data_req for ep0.

quoted

quoted

quoted

In fact, it is already at FSL i.mx internal code, just still not

mainlined.

quoted

quoted

so, do I get an Acked-by to this patch ? Does it need to go on
v3.6-rc or can it wait until v3.7 merge window ?

Without this (or the mentioned data_req patch), I can crash a g_multi
gadget by executing 'lsusb -v' as root on the host.  Should not be
exploitable (only a BUG_ON() is triggered) but issue should be fixed
asap.

=20
cool, so I'll apply to my fixes branch as soon as I get Acked-by or
Tested-by from someone.

This seems to revert the error handling for USB2.0 spec 8.5.3.3.  But the p=
roblem is a serious one to be fixed right away.  So

Acked-by: Li Yang <redacted>

We need to revisit the error handling issue later and find a proper way to =
address it.

Regards,
Leo