Re: [PATCH] usb: dwc2: gadget: avoid null dereference on incomplete transfer
Hi, John Youn writes: > On 3/30/2016 6:22 AM, Felipe Balbi wrote: >> >> Hi, >> >> John Keeping writes: >>> Setting up a gadget with the uac2 function results in: >>> >>> Unable to handle kernel NULL pointer dereference at virtual address >>> 0058 >>> ... >>> PC is at dwc2_hsotg_irq+0x7f0/0x908 >>> LR is at dwc2_hsotg_irq+0x4c/0x908 >>> Backtrace: >>> [] (dwc2_hsotg_irq) from [] >>> (handle_irq_event_percpu+0x130/0x3ec) >>> [] (handle_irq_event_percpu) from [] >>> (handle_irq_event+0x48/0x6c) >>> >>> In all other loops we already skip endpoints that are null, so do so >>> here as well. >>> >>> Signed-off-by: John Keeping >>> --- >>> drivers/usb/dwc2/gadget.c | 8 ++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>> index 0abf73c..df43ec0 100644 >>> --- a/drivers/usb/dwc2/gadget.c >>> +++ b/drivers/usb/dwc2/gadget.c >>> @@ -2606,7 +2606,9 @@ irq_retry: >>> for (idx = 1; idx < hsotg->num_of_eps; idx++) { >>> hs_ep = hsotg->eps_in[idx]; >>> >>> - if (!hs_ep->isochronous || hs_ep->has_correct_parity) >>> + if (!hs_ep || >>> + !hs_ep->isochronous || >>> + hs_ep->has_correct_parity) >> >> this is fine (even though choice of where to break line is a bit odd), >> but I have a question about how the rest of the code works (a bit >> off-topic, sorry) >> >>> continue; >>> >>> epctl_reg = DIEPCTL(idx); >> >> So, this means that the first ISO endpoint without correct parity will >> be used. Isn't this a bit fragile ? What happens when you use a device >> with several different interfaces using several different endpoints ? >> >> Isn't there a register where we can check which physical endpoint >> generated the IRQ ? Seems like you really wanna check what: >> > > We discussed this back when the patch was first submitted and > determined it should work fine like this. I don't remember exactly why > though. > > But this ISOC parity stuff is a workaround and we have a series of > patches to correctly set up ISOC allowing us to remove it. We're doing > some final tests before we send them. fair enough, thanks -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc2: gadget: avoid null dereference on incomplete transfer
On 3/30/2016 6:22 AM, Felipe Balbi wrote: > > Hi, > > John Keeping writes: >> Setting up a gadget with the uac2 function results in: >> >> Unable to handle kernel NULL pointer dereference at virtual address >> 0058 >> ... >> PC is at dwc2_hsotg_irq+0x7f0/0x908 >> LR is at dwc2_hsotg_irq+0x4c/0x908 >> Backtrace: >> [] (dwc2_hsotg_irq) from [] >> (handle_irq_event_percpu+0x130/0x3ec) >> [] (handle_irq_event_percpu) from [] >> (handle_irq_event+0x48/0x6c) >> >> In all other loops we already skip endpoints that are null, so do so >> here as well. >> >> Signed-off-by: John Keeping >> --- >> drivers/usb/dwc2/gadget.c | 8 ++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 0abf73c..df43ec0 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -2606,7 +2606,9 @@ irq_retry: >> for (idx = 1; idx < hsotg->num_of_eps; idx++) { >> hs_ep = hsotg->eps_in[idx]; >> >> -if (!hs_ep->isochronous || hs_ep->has_correct_parity) >> +if (!hs_ep || >> +!hs_ep->isochronous || >> +hs_ep->has_correct_parity) > > this is fine (even though choice of where to break line is a bit odd), > but I have a question about how the rest of the code works (a bit > off-topic, sorry) > >> continue; >> >> epctl_reg = DIEPCTL(idx); > > So, this means that the first ISO endpoint without correct parity will > be used. Isn't this a bit fragile ? What happens when you use a device > with several different interfaces using several different endpoints ? > > Isn't there a register where we can check which physical endpoint > generated the IRQ ? Seems like you really wanna check what: > We discussed this back when the patch was first submitted and determined it should work fine like this. I don't remember exactly why though. But this ISOC parity stuff is a workaround and we have a series of patches to correctly set up ISOC allowing us to remove it. We're doing some final tests before we send them. Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc2: gadget: avoid null dereference on incomplete transfer
Hi, John Keeping writes: > Setting up a gadget with the uac2 function results in: > > Unable to handle kernel NULL pointer dereference at virtual address 0058 > ... > PC is at dwc2_hsotg_irq+0x7f0/0x908 > LR is at dwc2_hsotg_irq+0x4c/0x908 > Backtrace: > [] (dwc2_hsotg_irq) from [] > (handle_irq_event_percpu+0x130/0x3ec) > [] (handle_irq_event_percpu) from [] > (handle_irq_event+0x48/0x6c) > > In all other loops we already skip endpoints that are null, so do so > here as well. > > Signed-off-by: John Keeping > --- > drivers/usb/dwc2/gadget.c | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 0abf73c..df43ec0 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -2606,7 +2606,9 @@ irq_retry: > for (idx = 1; idx < hsotg->num_of_eps; idx++) { > hs_ep = hsotg->eps_in[idx]; > > - if (!hs_ep->isochronous || hs_ep->has_correct_parity) > + if (!hs_ep || > + !hs_ep->isochronous || > + hs_ep->has_correct_parity) this is fine (even though choice of where to break line is a bit odd), but I have a question about how the rest of the code works (a bit off-topic, sorry) > continue; > > epctl_reg = DIEPCTL(idx); So, this means that the first ISO endpoint without correct parity will be used. Isn't this a bit fragile ? What happens when you use a device with several different interfaces using several different endpoints ? Isn't there a register where we can check which physical endpoint generated the IRQ ? Seems like you really wanna check what: #define DIEPINT(_a) HSOTG_REG(0x908 + ((_a) * 0x20)) say about eps_in[idx]. > @@ -2623,7 +2625,9 @@ irq_retry: > for (idx = 1; idx < hsotg->num_of_eps; idx++) { > hs_ep = hsotg->eps_out[idx]; > > - if (!hs_ep->isochronous || hs_ep->has_correct_parity) > + if (!hs_ep || > + !hs_ep->isochronous || > + hs_ep->has_correct_parity) > continue; > > epctl_reg = DOEPCTL(idx); ditto for eps_out[idx] and: #define DOEPINT(_a) HSOTG_REG(0xB08 + ((_a) * 0x20)) comments ? -- balbi signature.asc Description: PGP signature
[PATCH] usb: dwc2: gadget: avoid null dereference on incomplete transfer
Setting up a gadget with the uac2 function results in: Unable to handle kernel NULL pointer dereference at virtual address 0058 ... PC is at dwc2_hsotg_irq+0x7f0/0x908 LR is at dwc2_hsotg_irq+0x4c/0x908 Backtrace: [] (dwc2_hsotg_irq) from [] (handle_irq_event_percpu+0x130/0x3ec) [] (handle_irq_event_percpu) from [] (handle_irq_event+0x48/0x6c) In all other loops we already skip endpoints that are null, so do so here as well. Signed-off-by: John Keeping --- drivers/usb/dwc2/gadget.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 0abf73c..df43ec0 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -2606,7 +2606,9 @@ irq_retry: for (idx = 1; idx < hsotg->num_of_eps; idx++) { hs_ep = hsotg->eps_in[idx]; - if (!hs_ep->isochronous || hs_ep->has_correct_parity) + if (!hs_ep || + !hs_ep->isochronous || + hs_ep->has_correct_parity) continue; epctl_reg = DIEPCTL(idx); @@ -2623,7 +2625,9 @@ irq_retry: for (idx = 1; idx < hsotg->num_of_eps; idx++) { hs_ep = hsotg->eps_out[idx]; - if (!hs_ep->isochronous || hs_ep->has_correct_parity) + if (!hs_ep || + !hs_ep->isochronous || + hs_ep->has_correct_parity) continue; epctl_reg = DOEPCTL(idx); -- 2.7.0.226.gfe986fe -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html