* [PATCH] drivers: usb: gadget: udc: Fix NULL dereference @ 2015-03-03 12:58 Tapasweni Pathak 2015-03-04 1:11 ` Peter Chen 0 siblings, 1 reply; 7+ messages in thread From: Tapasweni Pathak @ 2015-03-03 12:58 UTC (permalink / raw) To: balbi, gregkh, peter.chen, jg1.han, benoit.taine, linux-usb, linux-kernel Cc: julia.lawall, tapaswenipathak 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 <tapaswenipathak@gmail.com> Suggested-by : Julia Lawall <julia.lawall@lip6.fr> Reviewed-by : Julia Lawall <julia.lawall@lip6.fr> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference 2015-03-03 12:58 [PATCH] drivers: usb: gadget: udc: Fix NULL dereference Tapasweni Pathak @ 2015-03-04 1:11 ` Peter Chen 2015-03-09 15:41 ` Felipe Balbi 0 siblings, 1 reply; 7+ messages in thread From: Peter Chen @ 2015-03-04 1:11 UTC (permalink / raw) To: Tapasweni Pathak Cc: balbi, gregkh, jg1.han, benoit.taine, linux-usb, linux-kernel, julia.lawall 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 <tapaswenipathak@gmail.com> > Suggested-by : Julia Lawall <julia.lawall@lip6.fr> > Reviewed-by : Julia Lawall <julia.lawall@lip6.fr> > --- > 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference 2015-03-04 1:11 ` Peter Chen @ 2015-03-09 15:41 ` Felipe Balbi 2015-03-10 2:02 ` Peter Chen 0 siblings, 1 reply; 7+ messages in thread From: Felipe Balbi @ 2015-03-09 15:41 UTC (permalink / raw) To: Peter Chen Cc: Tapasweni Pathak, balbi, gregkh, jg1.han, benoit.taine, linux-usb, linux-kernel, julia.lawall [-- Attachment #1: Type: text/plain, Size: 2886 bytes --] 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 <tapaswenipathak@gmail.com> > > Suggested-by : Julia Lawall <julia.lawall@lip6.fr> > > Reviewed-by : Julia Lawall <julia.lawall@lip6.fr> > > --- > > 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference 2015-03-09 15:41 ` Felipe Balbi @ 2015-03-10 2:02 ` Peter Chen 2015-03-10 2:36 ` Felipe Balbi 0 siblings, 1 reply; 7+ messages in thread From: Peter Chen @ 2015-03-10 2:02 UTC (permalink / raw) To: balbi Cc: Tapasweni Pathak, gregkh, jg1.han, benoit.taine, linux-usb, linux-kernel, julia.lawall > > --- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference 2015-03-10 2:02 ` Peter Chen @ 2015-03-10 2:36 ` Felipe Balbi 2015-03-10 3:00 ` Peter Chen 0 siblings, 1 reply; 7+ messages in thread From: Felipe Balbi @ 2015-03-10 2:36 UTC (permalink / raw) To: Peter Chen Cc: balbi, Tapasweni Pathak, gregkh, jg1.han, benoit.taine, linux-usb, linux-kernel, julia.lawall [-- Attachment #1: Type: text/plain, Size: 1202 bytes --] On Tue, Mar 10, 2015 at 02:02:44AM +0000, 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference 2015-03-10 2:36 ` Felipe Balbi @ 2015-03-10 3:00 ` Peter Chen 2015-03-10 20:50 ` Felipe Balbi 0 siblings, 1 reply; 7+ messages in thread From: Peter Chen @ 2015-03-10 3:00 UTC (permalink / raw) To: balbi Cc: Tapasweni Pathak, gregkh, jg1.han, benoit.taine, linux-usb, linux-kernel, julia.lawall > On Tue, Mar 10, 2015 at 02:02:44AM +0000, 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference 2015-03-10 3:00 ` Peter Chen @ 2015-03-10 20:50 ` Felipe Balbi 0 siblings, 0 replies; 7+ messages in thread From: Felipe Balbi @ 2015-03-10 20:50 UTC (permalink / raw) To: Peter Chen Cc: balbi, Tapasweni Pathak, gregkh, jg1.han, benoit.taine, linux-usb, linux-kernel, julia.lawall [-- Attachment #1: Type: text/plain, Size: 1495 bytes --] On Tue, Mar 10, 2015 at 03:00:49AM +0000, Peter Chen wrote: > > > On Tue, Mar 10, 2015 at 02:02:44AM +0000, 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-03-10 20:51 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-03 12:58 [PATCH] drivers: usb: gadget: udc: Fix NULL dereference Tapasweni Pathak 2015-03-04 1:11 ` Peter Chen 2015-03-09 15:41 ` Felipe Balbi 2015-03-10 2:02 ` Peter Chen 2015-03-10 2:36 ` Felipe Balbi 2015-03-10 3:00 ` Peter Chen 2015-03-10 20:50 ` Felipe Balbi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).