| Message ID | bbb1564aa649a6b5b97160ec3ef9fefdd8c85aea.1574891043.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: gadget: Check for NULL descriptor | expand |
On Wed, Nov 27, 2019 at 01:45:15PM -0800, Thinh Nguyen wrote: > The function driver may try to enable an unconfigured endpoint. This > check make sure that we do not attempt to access a NULL descriptor and > crash. > > Cc: stable@vger.kernel.org > Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > --- > drivers/usb/dwc3/gadget.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 7f97856e6b20..00f8f079bbf2 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -619,6 +619,9 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action) > u32 reg; > int ret; > > + if (!desc) > + return -EINVAL; How can this happen? Shouldn't this be caught at an earlier point in time? thanks, greg k-h
Greg Kroah-Hartman wrote: > On Wed, Nov 27, 2019 at 01:45:15PM -0800, Thinh Nguyen wrote: >> The function driver may try to enable an unconfigured endpoint. This >> check make sure that we do not attempt to access a NULL descriptor and >> crash. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >> --- >> drivers/usb/dwc3/gadget.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 7f97856e6b20..00f8f079bbf2 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -619,6 +619,9 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action) >> u32 reg; >> int ret; >> >> + if (!desc) >> + return -EINVAL; > How can this happen? Shouldn't this be caught at an earlier point in > time? Yeah, it should, and it's already handled or noted in all the function drivers in the kernel. It just bugs me a little seeing that it doesn't fail gracefully if it's not the case. You can discard this patch if you think it's unnecessary. Thanks, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > The function driver may try to enable an unconfigured endpoint. This > check make sure that we do not attempt to access a NULL descriptor and > crash. > > Cc: stable@vger.kernel.org > Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > --- > drivers/usb/dwc3/gadget.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 7f97856e6b20..00f8f079bbf2 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -619,6 +619,9 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action) > u32 reg; > int ret; > > + if (!desc) > + return -EINVAL; I would rather have a dev_WARN() (and return -EINVAL) added to usb_ep_enable() so we catch those doing this. That way we don't have to patch every UDC.
Hi Felipe, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > >> The function driver may try to enable an unconfigured endpoint. This >> check make sure that we do not attempt to access a NULL descriptor and >> crash. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >> --- >> drivers/usb/dwc3/gadget.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 7f97856e6b20..00f8f079bbf2 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -619,6 +619,9 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action) >> u32 reg; >> int ret; >> >> + if (!desc) >> + return -EINVAL; > I would rather have a dev_WARN() (and return -EINVAL) added to > usb_ep_enable() so we catch those doing this. That way we don't have to > patch every UDC. > Sure, we can do that. Thanks, Thinh
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 7f97856e6b20..00f8f079bbf2 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -619,6 +619,9 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action) u32 reg; int ret; + if (!desc) + return -EINVAL; + if (!(dep->flags & DWC3_EP_ENABLED)) { ret = dwc3_gadget_start_config(dep); if (ret)
The function driver may try to enable an unconfigured endpoint. This check make sure that we do not attempt to access a NULL descriptor and crash. Cc: stable@vger.kernel.org Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> --- drivers/usb/dwc3/gadget.c | 3 +++ 1 file changed, 3 insertions(+)