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

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

[Enrico Scholz]

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 <enrico.scholz@sigma-chemnitz.de>
---
 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

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

[Chen Peter-B29397]

=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

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

[Felipe Balbi]

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

On Wed, Sep 05, 2012 at 02:10:39AM +0000, Chen Peter-B29397 wrote:
>  
> > 
> > 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Enrico Scholz]

Felipe Balbi <balbi@ti.com> writes:

>> > 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

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

[Felipe Balbi]

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

Hi,

On Thu, Sep 06, 2012 at 04:27:12PM +0200, Enrico Scholz wrote:
> Felipe Balbi <balbi@ti.com> writes:
> 
> >> > 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

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

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:
> > Felipe Balbi <balbi@ti.com> writes:
> > 
> > >> > 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Li Yang-R58472]

> -----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:
> > Felipe Balbi <balbi@ti.com> writes:
> >
> > >> > 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.
> > >> >
> > >> > 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.
>=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 <leoli@freescale.com>

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

Regards,
Leo