From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753581AbbCIPmq (ORCPT ); Mon, 9 Mar 2015 11:42:46 -0400 Received: from devils.ext.ti.com ([198.47.26.153]:40830 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751664AbbCIPmn (ORCPT ); Mon, 9 Mar 2015 11:42:43 -0400 Date: Mon, 9 Mar 2015 10:41:56 -0500 From: Felipe Balbi To: Peter Chen CC: Tapasweni Pathak , , , , , , , Subject: Re: [PATCH] drivers: usb: gadget: udc: Fix NULL dereference Message-ID: <20150309154156.GB3739@saruman.tx.rr.com> Reply-To: References: <20150303125841.GA9671@kt-Inspiron-3542> <20150304011118.GB23399@shlinux2> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="qcHopEYAB45HaUaB" Content-Disposition: inline In-Reply-To: <20150304011118.GB23399@shlinux2> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --qcHopEYAB45HaUaB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >=20 > > 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, mak= ing > > an invalid pointer reference. > >=20 > > udc is then checked for NULL, if NULL, it is then dereferenced as > > udc->dev. > >=20 > > To fix these issues, shift assignment of udc by dereferencing ep after > > null check for _ep, replace both dev_dbg statements with pr_debug. > >=20 > > Found using Coccinelle. > >=20 > > 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(-) > >=20 > > 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; > >=20 > > - udc =3D ep->udc; > > - > > if (!_ep) { > > - dev_dbg(udc->dev, "invalid ep\n"); > > + pr_debug("invalid ep\n"); > > return -EINVAL; > > } > >=20 > > + udc =3D ep->udc; > >=20 > > if ((!udc) || (!udc->driver) || > > (udc->gadget.speed =3D=3D USB_SPEED_UNKNOWN)) { > > - dev_dbg(udc->dev, "invalid device\n"); > > + pr_debug("invalid device\n"); > > return -EINVAL; > > } > >=20 >=20 > 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: >=20 > diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/ud= c/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 =3D container_of(_req, struct lpc32xx_request, req); > ep =3D container_of(_ep, struct lpc32xx_ep, ep); > =20 > - if (!_req || !_req->complete || !_req->buf || > + if (!_ep || !_req || !_req->complete || !_req->buf || > !list_empty(&req->queue)) > return -EINVAL; > =20 > @@ -1815,8 +1815,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep, > } > =20 > =20 > - if ((!udc) || (!udc->driver) || > - (udc->gadget.speed =3D=3D USB_SPEED_UNKNOWN)) { > + if ((!udc->driver) || (udc->gadget.speed =3D=3D USB_SPEED_UNKNOWN)) { > dev_dbg(udc->dev, "invalid device\n"); > return -EINVAL; > } what's going to happen here ? --=20 balbi --qcHopEYAB45HaUaB Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJU/b9EAAoJEIaOsuA1yqREvE0QAKSTMIGCeos3c2+5SzqfuVLP AHUUX4qh22RmILMdeAwQ6xYnf2Qhzw/Pj28VamUzVARw9ueMG6002fOIXYlGz0PV UZaW9ZPo4AkZKhcAqWbgMVpVuUdeSXStBIxxLtIXJCBVmEo+N7zO8APgOR/LOtYi 4yVdk3VHYbhhdI65agxyQRkV6aJXd0iIZpqEMsYVN5E7rio1S2lBlwHGutgDLBhK 7CDZBla1+vgLu+4E5VM12YVPSUndfV0N7mp59+SB3BroYa7uS6R/u8LG5Cj/4Nh5 j1wm4SKyYltBzDY53yiWwYAc7V/SYVMM1scHbx90XNfe9/vaFTys0yyjeWVntrpx MsyKToY5trOMqz/RTGYNutNt4au+JDJnfd7G+SEFmAzRdP13tQJVDJiujeLtK6/5 OH0ewmpuCdOQ6Wy7/yNeyDTtN6wg4bX0L0mj2Z2MZqKF0GlPGmbPFM7v6W7nv4IC 0Ee0ouY4QdPUzbthq1AZg4rBAJ0OhoBzRBTrl1jovvpPQdwBrgSYB10fy+2CVGEX bb6Nfzmg3RP8abc4VQcnBG9/wMqQqNVaGPG0YS1tXZib2C6oSD0/4Ps/qsd5JjS9 blYCU4XYfbgAK/0OiPscNKN/DZw/cxtDTNwzfKOgiZ0TnuFwUDIoAMQ7DeD1U7Uy JhK3skzKONKBa5j/DS/e =eQtL -----END PGP SIGNATURE----- --qcHopEYAB45HaUaB--