| Message ID | 1346777932-3362-1-git-send-email-enrico.scholz@sigma-chemnitz.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
> > 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. Peter
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 ?
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
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
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 ?
> -----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 > > 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. This seems to revert the error handling for USB2.0 spec 8.5.3.3. But the problem 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
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 */
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(-)