* [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference @ 2015-09-04 11:42 Sudip Mukherjee 2015-09-10 18:03 ` David Cohen 0 siblings, 1 reply; 6+ messages in thread From: Sudip Mukherjee @ 2015-09-04 11:42 UTC (permalink / raw) To: Thomas Dahlmann, Felipe Balbi, Greg Kroah-Hartman Cc: linux-kernel, linux-geode, linux-usb, Sudip Mukherjee 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 <sudip@vectorindia.org> --- 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) } /* reset controller */ - writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg); + if (dev->regs) { + writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg); + iounmap(dev->regs); + } if (dev->irq_registered) free_irq(pdev->irq, dev); - if (dev->regs) - iounmap(dev->regs); if (dev->mem_region) release_mem_region(pci_resource_start(pdev, 0), pci_resource_len(pdev, 0)); -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference 2015-09-04 11:42 [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference Sudip Mukherjee @ 2015-09-10 18:03 ` David Cohen 2015-09-11 10:02 ` Sudip Mukherjee 0 siblings, 1 reply; 6+ messages in thread From: David Cohen @ 2015-09-10 18:03 UTC (permalink / raw) To: Sudip Mukherjee Cc: Thomas Dahlmann, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, linux-geode, linux-usb 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 <sudip@vectorindia.org> > --- > 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) > } > > /* reset controller */ > - writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg); > + if (dev->regs) { > + writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg); > + iounmap(dev->regs); I'm not familiar with the driver, but you're iounmap'ing before freeing irq. Looks fishy to me. Br, David > + } > if (dev->irq_registered) > free_irq(pdev->irq, dev); > - if (dev->regs) > - iounmap(dev->regs); > if (dev->mem_region) > release_mem_region(pci_resource_start(pdev, 0), > pci_resource_len(pdev, 0)); > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference 2015-09-10 18:03 ` David Cohen @ 2015-09-11 10:02 ` Sudip Mukherjee 2015-09-11 13:28 ` Felipe Balbi 0 siblings, 1 reply; 6+ messages in thread From: Sudip Mukherjee @ 2015-09-11 10:02 UTC (permalink / raw) To: David Cohen Cc: Thomas Dahlmann, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, linux-geode, linux-usb 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 <sudip@vectorindia.org> > > --- > > 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) <snip> > > 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference 2015-09-11 10:02 ` Sudip Mukherjee @ 2015-09-11 13:28 ` Felipe Balbi 2015-09-11 14:21 ` Sudip Mukherjee 0 siblings, 1 reply; 6+ messages in thread From: Felipe Balbi @ 2015-09-11 13:28 UTC (permalink / raw) To: Sudip Mukherjee Cc: David Cohen, Thomas Dahlmann, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, linux-geode, linux-usb [-- Attachment #1: Type: text/plain, Size: 2965 bytes --] 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, > > > > 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 <sudip@vectorindia.org> > > > --- > > > 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) > <snip> > > > > 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); 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) > > 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 -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference 2015-09-11 13:28 ` Felipe Balbi @ 2015-09-11 14:21 ` Sudip Mukherjee 2015-09-11 15:05 ` Felipe Balbi 0 siblings, 1 reply; 6+ messages in thread From: Sudip Mukherjee @ 2015-09-11 14:21 UTC (permalink / raw) To: Felipe Balbi Cc: David Cohen, Thomas Dahlmann, Greg Kroah-Hartman, linux-kernel, linux-geode, linux-usb On Fri, Sep 11, 2015 at 08:28:34AM -0500, Felipe Balbi wrote: > 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: <snip> > 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. :( Yes, total mess. I am on it. BTW, while I am fixing it can i also include a patch which will rearrange the functions and remove the forward declarations? regards sudip ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference 2015-09-11 14:21 ` Sudip Mukherjee @ 2015-09-11 15:05 ` Felipe Balbi 0 siblings, 0 replies; 6+ messages in thread From: Felipe Balbi @ 2015-09-11 15:05 UTC (permalink / raw) To: Sudip Mukherjee Cc: Felipe Balbi, David Cohen, Thomas Dahlmann, Greg Kroah-Hartman, linux-kernel, linux-geode, linux-usb [-- Attachment #1: Type: text/plain, Size: 1300 bytes --] On Fri, Sep 11, 2015 at 07:51:01PM +0530, Sudip Mukherjee wrote: > On Fri, Sep 11, 2015 at 08:28:34AM -0500, Felipe Balbi wrote: > > 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: > <snip> > > 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. > :( > Yes, total mess. I am on it. > BTW, while I am fixing it can i also include a patch which will > rearrange the functions and remove the forward declarations? sure, just make sure fixes and cleanups are separate. First all fixes, then cleanups and reorderings. cheers -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-11 15:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-04 11:42 [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference Sudip Mukherjee 2015-09-10 18:03 ` David Cohen 2015-09-11 10:02 ` Sudip Mukherjee 2015-09-11 13:28 ` Felipe Balbi 2015-09-11 14:21 ` Sudip Mukherjee 2015-09-11 15:05 ` Felipe Balbi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox