Re: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference
On Tue, Mar 10, 2015 at 03:00:49AM +, Peter Chen wrote: > > > On Tue, Mar 10, 2015 at 02:02:44AM +, Peter Chen wrote: > > > > > > > > --- a/drivers/usb/gadget/udc/lpc32xx_udc.c > > > > > +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c > > > > > @@ -1803,7 +1803,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > > > > > req = container_of(_req, struct lpc32xx_request, req); > > > > > ep = container_of(_ep, struct lpc32xx_ep, ep); > > > > > > > > > > - if (!_req || !_req->complete || !_req->buf || > > > > > + if (!_ep || !_req || !_req->complete || !_req->buf || > > > > > !list_empty(&req->queue)) > > > > > return -EINVAL; > > > > > > > > > > @@ -1815,8 +1815,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > > > > > } > > > > > > > > > > > > > > > - if ((!udc) || (!udc->driver) || > > > > > - (udc->gadget.speed == USB_SPEED_UNKNOWN)) { > > > > > + if ((!udc->driver) || (udc->gadget.speed == USB_SPEED_UNKNOWN)) > > > > { > > > > > dev_dbg(udc->dev, "invalid device\n"); > > > > > return -EINVAL; > > > > > } > > > > > > > > what's going to happen here ? > > > > > > > > > > I just changed the current code, in fact, udc->driver is impossible to > > > NULL which is cleared at .udc_stop. > > > > > > The speed is possible for unknown if the reset has occurred at that time. > > > > oh, alright. > > Do you need me or Tapasweni send patch for this? if there's anything to be fixed, sure. -- balbi signature.asc Description: Digital signature
RE: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference
> On Tue, Mar 10, 2015 at 02:02:44AM +, Peter Chen wrote: > > > > > > --- a/drivers/usb/gadget/udc/lpc32xx_udc.c > > > > +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c > > > > @@ -1803,7 +1803,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > > > > req = container_of(_req, struct lpc32xx_request, req); > > > > ep = container_of(_ep, struct lpc32xx_ep, ep); > > > > > > > > - if (!_req || !_req->complete || !_req->buf || > > > > + if (!_ep || !_req || !_req->complete || !_req->buf || > > > > !list_empty(&req->queue)) > > > > return -EINVAL; > > > > > > > > @@ -1815,8 +1815,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > > > > } > > > > > > > > > > > > - if ((!udc) || (!udc->driver) || > > > > - (udc->gadget.speed == USB_SPEED_UNKNOWN)) { > > > > + if ((!udc->driver) || (udc->gadget.speed == USB_SPEED_UNKNOWN)) > > > { > > > > dev_dbg(udc->dev, "invalid device\n"); > > > > return -EINVAL; > > > > } > > > > > > what's going to happen here ? > > > > > > > I just changed the current code, in fact, udc->driver is impossible to > > NULL which is cleared at .udc_stop. > > > > The speed is possible for unknown if the reset has occurred at that time. > > oh, alright. Do you need me or Tapasweni send patch for this? Peter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference
On Tue, Mar 10, 2015 at 02:02:44AM +, Peter Chen wrote: > > > > --- a/drivers/usb/gadget/udc/lpc32xx_udc.c > > > +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c > > > @@ -1803,7 +1803,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > > > req = container_of(_req, struct lpc32xx_request, req); > > > ep = container_of(_ep, struct lpc32xx_ep, ep); > > > > > > - if (!_req || !_req->complete || !_req->buf || > > > + if (!_ep || !_req || !_req->complete || !_req->buf || > > > !list_empty(&req->queue)) > > > return -EINVAL; > > > > > > @@ -1815,8 +1815,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > > > } > > > > > > > > > - if ((!udc) || (!udc->driver) || > > > - (udc->gadget.speed == USB_SPEED_UNKNOWN)) { > > > + if ((!udc->driver) || (udc->gadget.speed == USB_SPEED_UNKNOWN)) > > { > > > dev_dbg(udc->dev, "invalid device\n"); > > > return -EINVAL; > > > } > > > > what's going to happen here ? > > > > I just changed the current code, in fact, udc->driver is impossible to NULL > which > is cleared at .udc_stop. > > The speed is possible for unknown if the reset has occurred at that time. oh, alright. -- balbi signature.asc Description: Digital signature
RE: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference
> > --- a/drivers/usb/gadget/udc/lpc32xx_udc.c > > +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c > > @@ -1803,7 +1803,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > > req = container_of(_req, struct lpc32xx_request, req); > > ep = container_of(_ep, struct lpc32xx_ep, ep); > > > > - if (!_req || !_req->complete || !_req->buf || > > + if (!_ep || !_req || !_req->complete || !_req->buf || > > !list_empty(&req->queue)) > > return -EINVAL; > > > > @@ -1815,8 +1815,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > > } > > > > > > - if ((!udc) || (!udc->driver) || > > - (udc->gadget.speed == USB_SPEED_UNKNOWN)) { > > + if ((!udc->driver) || (udc->gadget.speed == USB_SPEED_UNKNOWN)) > { > > dev_dbg(udc->dev, "invalid device\n"); > > return -EINVAL; > > } > > what's going to happen here ? > I just changed the current code, in fact, udc->driver is impossible to NULL which is cleared at .udc_stop. The speed is possible for unknown if the reset has occurred at that time. Peter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference
On Wed, Mar 04, 2015 at 09:11:19AM +0800, Peter Chen wrote: > On Tue, Mar 03, 2015 at 06:28:42PM +0530, Tapasweni Pathak wrote: > > This patch fixes multiple instances of null pointer dereference in this > > code. > > > > ep->udc is assigned to udc. ep is just an offset from _ep. _ep is then > > checked for NULL. udc is dereferenced under the NULL check for _ep, making > > an invalid pointer reference. > > > > udc is then checked for NULL, if NULL, it is then dereferenced as > > udc->dev. > > > > To fix these issues, shift assignment of udc by dereferencing ep after > > null check for _ep, replace both dev_dbg statements with pr_debug. > > > > Found using Coccinelle. > > > > Signed-off-by: Tapasweni Pathak > > Suggested-by : Julia Lawall > > Reviewed-by : Julia Lawall > > --- > > drivers/usb/gadget/udc/lpc32xx_udc.c |7 +++ > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c > > b/drivers/usb/gadget/udc/lpc32xx_udc.c > > index 27fd413..6398539 100644 > > --- a/drivers/usb/gadget/udc/lpc32xx_udc.c > > +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c > > @@ -1807,17 +1807,16 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > > !list_empty(&req->queue)) > > return -EINVAL; > > > > - udc = ep->udc; > > - > > if (!_ep) { > > - dev_dbg(udc->dev, "invalid ep\n"); > > + pr_debug("invalid ep\n"); > > return -EINVAL; > > } > > > > + udc = ep->udc; > > > > if ((!udc) || (!udc->driver) || > > (udc->gadget.speed == USB_SPEED_UNKNOWN)) { > > - dev_dbg(udc->dev, "invalid device\n"); > > + pr_debug("invalid device\n"); > > return -EINVAL; > > } > > > > It is driver code, we'd better use dev_dbg. If _ep exists, > both ep and udc should exist. So, how about just checking > _ep at the beginning: > > diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c > b/drivers/usb/gadget/udc/lpc32xx_udc.c > index 27fd413..c65de0e 100644 > --- a/drivers/usb/gadget/udc/lpc32xx_udc.c > +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c > @@ -1803,7 +1803,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > req = container_of(_req, struct lpc32xx_request, req); > ep = container_of(_ep, struct lpc32xx_ep, ep); > > - if (!_req || !_req->complete || !_req->buf || > + if (!_ep || !_req || !_req->complete || !_req->buf || > !list_empty(&req->queue)) > return -EINVAL; > > @@ -1815,8 +1815,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > } > > > - if ((!udc) || (!udc->driver) || > - (udc->gadget.speed == USB_SPEED_UNKNOWN)) { > + if ((!udc->driver) || (udc->gadget.speed == USB_SPEED_UNKNOWN)) { > dev_dbg(udc->dev, "invalid device\n"); > return -EINVAL; > } what's going to happen here ? -- balbi signature.asc Description: Digital signature
Re: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference
On Tue, Mar 03, 2015 at 06:28:42PM +0530, Tapasweni Pathak wrote: > This patch fixes multiple instances of null pointer dereference in this code. > > ep->udc is assigned to udc. ep is just an offset from _ep. _ep is then > checked for NULL. udc is dereferenced under the NULL check for _ep, making > an invalid pointer reference. > > udc is then checked for NULL, if NULL, it is then dereferenced as > udc->dev. > > To fix these issues, shift assignment of udc by dereferencing ep after > null check for _ep, replace both dev_dbg statements with pr_debug. > > Found using Coccinelle. > > Signed-off-by: Tapasweni Pathak > Suggested-by : Julia Lawall > Reviewed-by : Julia Lawall > --- > drivers/usb/gadget/udc/lpc32xx_udc.c |7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c > b/drivers/usb/gadget/udc/lpc32xx_udc.c > index 27fd413..6398539 100644 > --- a/drivers/usb/gadget/udc/lpc32xx_udc.c > +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c > @@ -1807,17 +1807,16 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > !list_empty(&req->queue)) > return -EINVAL; > > - udc = ep->udc; > - > if (!_ep) { > - dev_dbg(udc->dev, "invalid ep\n"); > + pr_debug("invalid ep\n"); > return -EINVAL; > } > > + udc = ep->udc; > > if ((!udc) || (!udc->driver) || > (udc->gadget.speed == USB_SPEED_UNKNOWN)) { > - dev_dbg(udc->dev, "invalid device\n"); > + pr_debug("invalid device\n"); > return -EINVAL; > } > It is driver code, we'd better use dev_dbg. If _ep exists, both ep and udc should exist. So, how about just checking _ep at the beginning: diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c index 27fd413..c65de0e 100644 --- a/drivers/usb/gadget/udc/lpc32xx_udc.c +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c @@ -1803,7 +1803,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, req = container_of(_req, struct lpc32xx_request, req); ep = container_of(_ep, struct lpc32xx_ep, ep); - if (!_req || !_req->complete || !_req->buf || + if (!_ep || !_req || !_req->complete || !_req->buf || !list_empty(&req->queue)) return -EINVAL; @@ -1815,8 +1815,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, } - if ((!udc) || (!udc->driver) || - (udc->gadget.speed == USB_SPEED_UNKNOWN)) { + if ((!udc->driver) || (udc->gadget.speed == USB_SPEED_UNKNOWN)) { dev_dbg(udc->dev, "invalid device\n"); return -EINVAL; } -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] drivers: usb: gadget: udc: Fix NULL dereference
This patch fixes multiple instances of null pointer dereference in this code. ep->udc is assigned to udc. ep is just an offset from _ep. _ep is then checked for NULL. udc is dereferenced under the NULL check for _ep, making an invalid pointer reference. udc is then checked for NULL, if NULL, it is then dereferenced as udc->dev. To fix these issues, shift assignment of udc by dereferencing ep after null check for _ep, replace both dev_dbg statements with pr_debug. Found using Coccinelle. Signed-off-by: Tapasweni Pathak Suggested-by : Julia Lawall Reviewed-by : Julia Lawall --- drivers/usb/gadget/udc/lpc32xx_udc.c |7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c index 27fd413..6398539 100644 --- a/drivers/usb/gadget/udc/lpc32xx_udc.c +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c @@ -1807,17 +1807,16 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, !list_empty(&req->queue)) return -EINVAL; - udc = ep->udc; - if (!_ep) { - dev_dbg(udc->dev, "invalid ep\n"); + pr_debug("invalid ep\n"); return -EINVAL; } + udc = ep->udc; if ((!udc) || (!udc->driver) || (udc->gadget.speed == USB_SPEED_UNKNOWN)) { - dev_dbg(udc->dev, "invalid device\n"); + pr_debug("invalid device\n"); return -EINVAL; } -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/