From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751960AbbIKKDK (ORCPT ); Fri, 11 Sep 2015 06:03:10 -0400 Received: from mail-pa0-f43.google.com ([209.85.220.43]:33851 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751273AbbIKKDJ (ORCPT ); Fri, 11 Sep 2015 06:03:09 -0400 Date: Fri, 11 Sep 2015 15:32:56 +0530 From: Sudip Mukherjee To: David Cohen Cc: Thomas Dahlmann , Felipe Balbi , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, linux-geode@lists.infradead.org, linux-usb@vger.kernel.org Subject: Re: [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference Message-ID: <20150911100256.GA15689@sudip-pc> References: <1441366943-9390-1-git-send-email-sudipm.mukherjee@gmail.com> <20150910180334.GA10760@psi-dev26.jf.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150910180334.GA10760@psi-dev26.jf.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 10, 2015 at 11:03:34AM -0700, David Cohen wrote: > Hi Sudip, > > 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. > > > > Signed-off-by: Sudip Mukherjee > > --- > > drivers/usb/gadget/udc/amd5536udc.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > 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 *pdev) > > 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: 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); } - /* 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); if (dev->irq_registered) free_irq(pdev->irq, dev); if (dev->regs) 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 just at that moment one interrupt comes and the driver tries to access the io memory while servicing the irq. regards sudip