From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753245AbbIKN3F (ORCPT ); Fri, 11 Sep 2015 09:29:05 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:50796 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752130AbbIKN3D (ORCPT ); Fri, 11 Sep 2015 09:29:03 -0400 Date: Fri, 11 Sep 2015 08:28:34 -0500 From: Felipe Balbi To: Sudip Mukherjee CC: David Cohen , Thomas Dahlmann , Felipe Balbi , Greg Kroah-Hartman , , , Subject: Re: [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference Message-ID: <20150911132834.GB17326@saruman.tx.rr.com> Reply-To: References: <1441366943-9390-1-git-send-email-sudipm.mukherjee@gmail.com> <20150910180334.GA10760@psi-dev26.jf.intel.com> <20150911100256.GA15689@sudip-pc> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="98e8jtXdkpgskNou" Content-Disposition: inline In-Reply-To: <20150911100256.GA15689@sudip-pc> 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 --98e8jtXdkpgskNou Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Fri, Sep 11, 2015 at 03:32:56PM +0530, Sudip Mukherjee wrote: > On Thu, Sep 10, 2015 at 11:03:34AM -0700, David Cohen wrote: > > Hi Sudip, > >=20 > > On Fri, Sep 04, 2015 at 05:12:23PM +0530, Sudip Mukherjee wrote: > > > We were checking if dev->regs is NULL but it was done after > > > dereferencing it. Lets reset the controller and iounmap dev->regs only > > > if it is not NULL. > > > free_irq() does not need dev->regs, so unmaping it before freeing the > > > irq should not matter. > > >=20 > > > Signed-off-by: Sudip Mukherjee > > > --- > > > drivers/usb/gadget/udc/amd5536udc.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > >=20 > > > diff --git a/drivers/usb/gadget/udc/amd5536udc.c b/drivers/usb/gadget= /udc/amd5536udc.c > > > index fdacddb..26066d3 100644 > > > --- a/drivers/usb/gadget/udc/amd5536udc.c > > > +++ b/drivers/usb/gadget/udc/amd5536udc.c > > > @@ -3135,11 +3135,12 @@ static void udc_pci_remove(struct pci_dev *pd= ev) > > >=20 > > I'm not familiar with the driver, but you're iounmap'ing before freeing > > irq. Looks fishy to me. > Well, I thought you will be able to give me some idea about how fix it. :) > Then I guess we should be on the safe side and what about the following: >=20 >=20 > diff --git a/drivers/usb/gadget/udc/amd5536udc.c b/drivers/usb/gadget/udc= /amd5536udc.c > index fdacddb..82f36f6 100644 > --- a/drivers/usb/gadget/udc/amd5536udc.c > +++ b/drivers/usb/gadget/udc/amd5536udc.c > @@ -3134,8 +3134,9 @@ static void udc_pci_remove(struct pci_dev *pdev) > pci_pool_destroy(dev->stp_requests); > } > =20 > - /* reset controller */ > - writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg); > + if (dev->regs) > + /* reset controller */ > + writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg); no, the check is pointless. Most of these are. Just look at your probe() and you'll see that if dev->virt_addr is NULL (meaning ioremap_nocache() failed) you exit from probe() with error. The driver doesn't probe at all. So you can be sure that by remove, dev->regs is valid. BTW, if probe fails, you have a TON of leaked resources!! You don't kfree() dev, you don't pci_disable_device(), you don't release_mem_region(), you don't iounmap() virt_addr, you don't free_irq(). Also the iounmap() call in remove is wrong. You ioremapped dev->virt_addr but iounmap() dev->regs. Are you SURE that's ok ? Man, what a mess! You gotta fix that up. > if (dev->irq_registered) > free_irq(pdev->irq, dev); > if (dev->regs) >=20 > And just for my information: for a device what might happen if I iounmap > before I free the irq? One thing I can think of is that after iounmap what happens if an IRQ fires after you iounmap() but before you free_irq() ? > just at that moment one interrupt comes and the driver tries to access > the io memory while servicing the irq. there you go --=20 balbi --98e8jtXdkpgskNou Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV8tcCAAoJEIaOsuA1yqRECvcP/RDLq150N2D/4sBPLN8jCS7O dC7UAIRJo4GZ/Tz4BWHOnnGrPmZkgu9AOQKx8SLWBkD/YLiEC2kCBPDWPI++8WRV Y77V/l5isxBvS++E4xDjBeYb2AviQn7mhSrXvmpEDAv5i0PY7q43Fi/DhCTiA+lr ILFoDr5b6sobuM2xY+ZfkUHhy5xF1laNe5ZNaBCyNnG5WKZvuhFBE2zz6w/6o4Da 8BC+2I/OeI/9TgDe4R0FEv+w1mNjBknkO0Ovo/jz0pJDtwYIMdDOYHd1fw6qVPj+ IC615r91RhxT7heGggmjXXjQBqAnr4uS0+uGweT6XqxWXlzrGUKy+2j8D87kWPPL HgiMePaFlNlK8MCWQcjnDzs6Kum3V6USqGGVcVoLpek1ddo0ZdftOEi54c0Rgj4b 5I4qhZxIy8s/cTWMikpHAQ4eR2pAo1xtSLfehxauSveIVes5XbChttKa/itzdkV5 9iY4GrU9iqVc+Rud+Hm8SRNjnFs+jWUFHzNCvN50K4Y1giTBeHu89zTZrx4eBt1L TrNW/NQmYb+qpfvA3clHnqO7IT25E0dI+244IsudG6o+j6kvIOSssRW/ZPic96+p 5zdEJxFYIfpOQ1mVJ4B2JEGo2AWIsUuKEpQOQ04eMunTclaKrhrla4eyLUZ3jD3L OHRUjC4K+uf2Wlh/stDc =PObh -----END PGP SIGNATURE----- --98e8jtXdkpgskNou--