[-next] PCI: dra7xx: Fix potential NULL dereference
diff mbox

Message ID 20180118183525.GG53542@bhelgaas-glaptop.roam.corp.google.com
State New
Headers show

Commit Message

Bjorn Helgaas Jan. 18, 2018, 6:35 p.m. UTC
On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote:
> On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote:
> > platform_get_resource_byname() may fail and return NULL, so we should
> > better check it's return value to avoid a NULL pointer dereference a
> > bit later in the code.
> > 
> > This is detected by Coccinelle semantic patch.
> > 
> > @@
> > expression pdev, res, n, t, e, e1, e2;
> > @@
> > 
> > res = platform_get_resource_byname(pdev, t, n);
> > + if (!res)
> > +   return -EINVAL;
> > ... when != res == NULL
> > e = devm_ioremap(e1, res->start, e2);
> 
> Well, then it should be replaced with devm_ioremap_resource()
> which already checks for NULL and the right resource type
> (IORESOURCE_MEM).

That's probably a better idea.  Maybe we should add a comment like this
to help avoid this in the future:


> > Fixes: 608793e27b33 ("PCI: dwc: dra7xx: Add EP mode support")
> > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> > ---
> >  drivers/pci/dwc/pci-dra7xx.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> > index 8bf7c27..aafded8 100644
> > --- a/drivers/pci/dwc/pci-dra7xx.c
> > +++ b/drivers/pci/dwc/pci-dra7xx.c
> > @@ -409,11 +409,15 @@ static int __init dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx,
> >  	ep->ops = &pcie_ep_ops;
> >  
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics");
> > +	if (!res)
> > +		return -EINVAL;
> >  	pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res));
> >  	if (!pci->dbi_base)
> >  		return -ENOMEM;
> >  
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics2");
> > +	if (!res)
> > +		return -EINVAL;
> >  	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
> >  	if (!pci->dbi_base2)
> >  		return -ENOMEM;
> > @@ -462,6 +466,8 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
> >  		return ret;
> >  
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbics");
> > +	if (!res)
> > +		return -EINVAL;
> >  	pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res));
> >  	if (!pci->dbi_base)
> >  		return -ENOMEM;
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ladislav Michl Jan. 18, 2018, 9:34 p.m. UTC | #1
On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote:
> > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote:
> > > platform_get_resource_byname() may fail and return NULL, so we should
> > > better check it's return value to avoid a NULL pointer dereference a
> > > bit later in the code.
> > > 
> > > This is detected by Coccinelle semantic patch.
> > > 
> > > @@
> > > expression pdev, res, n, t, e, e1, e2;
> > > @@
> > > 
> > > res = platform_get_resource_byname(pdev, t, n);
> > > + if (!res)
> > > +   return -EINVAL;
> > > ... when != res == NULL
> > > e = devm_ioremap(e1, res->start, e2);
> > 
> > Well, then it should be replaced with devm_ioremap_resource()
> > which already checks for NULL and the right resource type
> > (IORESOURCE_MEM).
> 
> That's probably a better idea.  Maybe we should add a comment like this
> to help avoid this in the future:
> 
> --- a/lib/devres.c
> +++ b/lib/devres.c
> @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev, void *res, void *match_data)
>   * @size: Size of map
>   *
>   * Managed ioremap().  Map is automatically unmapped on driver detach.
> + *
> + * When possible, use devm_ioremap_resource() instead.
>   */
>  void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>  			   resource_size_t size)

Yes, please. It would be nice first patch in the serie converting existing
users of devm_ioremap into devm_ioremap_resource:
find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size | wc -l
82
I know, that was dumb, Coccinelle would certainly do better job.
And from a quick look a lot of
if (!res) {
	print error
	return -EINVAL;
}
code blocks could be deleted (and many cases where check for NULL resource
is missing fixed).

> > > Fixes: 608793e27b33 ("PCI: dwc: dra7xx: Add EP mode support")
> > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> > > ---
> > >  drivers/pci/dwc/pci-dra7xx.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> > > index 8bf7c27..aafded8 100644
> > > --- a/drivers/pci/dwc/pci-dra7xx.c
> > > +++ b/drivers/pci/dwc/pci-dra7xx.c
> > > @@ -409,11 +409,15 @@ static int __init dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx,
> > >  	ep->ops = &pcie_ep_ops;
> > >  
> > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics");
> > > +	if (!res)
> > > +		return -EINVAL;
> > >  	pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res));
> > >  	if (!pci->dbi_base)
> > >  		return -ENOMEM;
> > >  
> > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics2");
> > > +	if (!res)
> > > +		return -EINVAL;
> > >  	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
> > >  	if (!pci->dbi_base2)
> > >  		return -ENOMEM;
> > > @@ -462,6 +466,8 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
> > >  		return ret;
> > >  
> > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbics");
> > > +	if (!res)
> > > +		return -EINVAL;
> > >  	pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res));
> > >  	if (!pci->dbi_base)
> > >  		return -ENOMEM;
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wei Yongjun Jan. 19, 2018, 1:54 a.m. UTC | #2
> On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote:
> > > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote:
> > > > platform_get_resource_byname() may fail and return NULL, so we
> should
> > > > better check it's return value to avoid a NULL pointer dereference a
> > > > bit later in the code.
> > > >
> > > > This is detected by Coccinelle semantic patch.
> > > >
> > > > @@
> > > > expression pdev, res, n, t, e, e1, e2;
> > > > @@
> > > >
> > > > res = platform_get_resource_byname(pdev, t, n);
> > > > + if (!res)
> > > > +   return -EINVAL;
> > > > ... when != res == NULL
> > > > e = devm_ioremap(e1, res->start, e2);
> > >
> > > Well, then it should be replaced with devm_ioremap_resource()
> > > which already checks for NULL and the right resource type
> > > (IORESOURCE_MEM).
> >
> > That's probably a better idea.  Maybe we should add a comment like this
> > to help avoid this in the future:

Not all of the place using devm_ioremap() can be replaced with
devm_ioremap_resource(), devices share the memory resource for example.

So maybe you should also add an exception list to the comment, otherwise
many people still not know how to use devm_ioremap_resource() or devm_ioremap().

> >
> > --- a/lib/devres.c
> > +++ b/lib/devres.c
> > @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev,
> void *res, void *match_data)
> >   * @size: Size of map
> >   *
> >   * Managed ioremap().  Map is automatically unmapped on driver detach.
> > + *
> > + * When possible, use devm_ioremap_resource() instead.
> >   */
> >  void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> >  			   resource_size_t size)
> 
> Yes, please. It would be nice first patch in the serie converting existing
> users of devm_ioremap into devm_ioremap_resource:
> find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size
> | wc -l
> 82
> I know, that was dumb, Coccinelle would certainly do better job.
> And from a quick look a lot of
> if (!res) {
> 	print error
> 	return -EINVAL;
> }
> code blocks could be deleted (and many cases where check for NULL
> resource
> is missing fixed).
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall Jan. 19, 2018, 5:56 a.m. UTC | #3
On Fri, 19 Jan 2018, weiyongjun (A) wrote:

> > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote:
> > > On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote:
> > > > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote:
> > > > > platform_get_resource_byname() may fail and return NULL, so we
> > should
> > > > > better check it's return value to avoid a NULL pointer dereference a
> > > > > bit later in the code.
> > > > >
> > > > > This is detected by Coccinelle semantic patch.
> > > > >
> > > > > @@
> > > > > expression pdev, res, n, t, e, e1, e2;
> > > > > @@
> > > > >
> > > > > res = platform_get_resource_byname(pdev, t, n);
> > > > > + if (!res)
> > > > > +   return -EINVAL;
> > > > > ... when != res == NULL
> > > > > e = devm_ioremap(e1, res->start, e2);
> > > >
> > > > Well, then it should be replaced with devm_ioremap_resource()
> > > > which already checks for NULL and the right resource type
> > > > (IORESOURCE_MEM).
> > >
> > > That's probably a better idea.  Maybe we should add a comment like this
> > > to help avoid this in the future:
>
> Not all of the place using devm_ioremap() can be replaced with
> devm_ioremap_resource(), devices share the memory resource for example.
>
> So maybe you should also add an exception list to the comment, otherwise
> many people still not know how to use devm_ioremap_resource() or devm_ioremap().

I believe that there is a semantic patch in the kernel to remove the test
when devm_ioremap_reource is used.  Maybe that should be extended or
another one should be added to ensure that there is a test when
devm_ioremap is used, since there seems to be a potential for confusion.

julia

>
> > >
> > > --- a/lib/devres.c
> > > +++ b/lib/devres.c
> > > @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev,
> > void *res, void *match_data)
> > >   * @size: Size of map
> > >   *
> > >   * Managed ioremap().  Map is automatically unmapped on driver detach.
> > > + *
> > > + * When possible, use devm_ioremap_resource() instead.
> > >   */
> > >  void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> > >  			   resource_size_t size)
> >
> > Yes, please. It would be nice first patch in the serie converting existing
> > users of devm_ioremap into devm_ioremap_resource:
> > find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size
> > | wc -l
> > 82
> > I know, that was dumb, Coccinelle would certainly do better job.
> > And from a quick look a lot of
> > if (!res) {
> > 	print error
> > 	return -EINVAL;
> > }
> > code blocks could be deleted (and many cases where check for NULL
> > resource
> > is missing fixed).
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ladislav Michl Jan. 19, 2018, 7:03 a.m. UTC | #4
On Fri, Jan 19, 2018 at 01:54:58AM +0000, weiyongjun (A) wrote:
> > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote:
> > > On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote:
> > > > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote:
> > > > > platform_get_resource_byname() may fail and return NULL, so we
> > should
> > > > > better check it's return value to avoid a NULL pointer dereference a
> > > > > bit later in the code.
> > > > >
> > > > > This is detected by Coccinelle semantic patch.
> > > > >
> > > > > @@
> > > > > expression pdev, res, n, t, e, e1, e2;
> > > > > @@
> > > > >
> > > > > res = platform_get_resource_byname(pdev, t, n);
> > > > > + if (!res)
> > > > > +   return -EINVAL;
> > > > > ... when != res == NULL
> > > > > e = devm_ioremap(e1, res->start, e2);
> > > >
> > > > Well, then it should be replaced with devm_ioremap_resource()
> > > > which already checks for NULL and the right resource type
> > > > (IORESOURCE_MEM).
> > >
> > > That's probably a better idea.  Maybe we should add a comment like this
> > > to help avoid this in the future:
> 
> Not all of the place using devm_ioremap() can be replaced with
> devm_ioremap_resource(), devices share the memory resource for example.

That's probably what "when possible" means. Also, how does sharing memory
resource changes that? As long as 'struct resource' is an argument to
devm_ioremap, devm_ioremap_resource can be used.

> So maybe you should also add an exception list to the comment, otherwise
> many people still not know how to use devm_ioremap_resource() or devm_ioremap().

Care to elaborate how should such an exception list look like?

Thank you.

> > >
> > > --- a/lib/devres.c
> > > +++ b/lib/devres.c
> > > @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev,
> > void *res, void *match_data)
> > >   * @size: Size of map
> > >   *
> > >   * Managed ioremap().  Map is automatically unmapped on driver detach.
> > > + *
> > > + * When possible, use devm_ioremap_resource() instead.
> > >   */
> > >  void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> > >  			   resource_size_t size)
> > 
> > Yes, please. It would be nice first patch in the serie converting existing
> > users of devm_ioremap into devm_ioremap_resource:
> > find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size
> > | wc -l
> > 82
> > I know, that was dumb, Coccinelle would certainly do better job.
> > And from a quick look a lot of
> > if (!res) {
> > 	print error
> > 	return -EINVAL;
> > }
> > code blocks could be deleted (and many cases where check for NULL
> > resource
> > is missing fixed).
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ladislav Michl Jan. 19, 2018, 9:16 a.m. UTC | #5
On Fri, Jan 19, 2018 at 08:03:38AM +0100, Ladislav Michl wrote:
> On Fri, Jan 19, 2018 at 01:54:58AM +0000, weiyongjun (A) wrote:
> > > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote:
> > > > On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote:
> > > > > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote:
> > > > > > platform_get_resource_byname() may fail and return NULL, so we
> > > should
> > > > > > better check it's return value to avoid a NULL pointer dereference a
> > > > > > bit later in the code.
> > > > > >
> > > > > > This is detected by Coccinelle semantic patch.
> > > > > >
> > > > > > @@
> > > > > > expression pdev, res, n, t, e, e1, e2;
> > > > > > @@
> > > > > >
> > > > > > res = platform_get_resource_byname(pdev, t, n);
> > > > > > + if (!res)
> > > > > > +   return -EINVAL;
> > > > > > ... when != res == NULL
> > > > > > e = devm_ioremap(e1, res->start, e2);
> > > > >
> > > > > Well, then it should be replaced with devm_ioremap_resource()
> > > > > which already checks for NULL and the right resource type
> > > > > (IORESOURCE_MEM).
> > > >
> > > > That's probably a better idea.  Maybe we should add a comment like this
> > > > to help avoid this in the future:
> > 
> > Not all of the place using devm_ioremap() can be replaced with
> > devm_ioremap_resource(), devices share the memory resource for example.
> 
> That's probably what "when possible" means. Also, how does sharing memory
> resource changes that? As long as 'struct resource' is an argument to
> devm_ioremap, devm_ioremap_resource can be used.
> 
> > So maybe you should also add an exception list to the comment, otherwise
> > many people still not know how to use devm_ioremap_resource() or devm_ioremap().
> 
> Care to elaborate how should such an exception list look like?

What about:
"When possible (for example when memory region was not already requested),
use devm_ioremap_resource() instead."?

And here I would propose something like devm_ioremap_resource_norequest()
To be honest, such a name looks too long for me, so suggestions welcome :)

> Thank you.
> 
> > > >
> > > > --- a/lib/devres.c
> > > > +++ b/lib/devres.c
> > > > @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev,
> > > void *res, void *match_data)
> > > >   * @size: Size of map
> > > >   *
> > > >   * Managed ioremap().  Map is automatically unmapped on driver detach.
> > > > + *
> > > > + * When possible, use devm_ioremap_resource() instead.
> > > >   */
> > > >  void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> > > >  			   resource_size_t size)
> > > 
> > > Yes, please. It would be nice first patch in the serie converting existing
> > > users of devm_ioremap into devm_ioremap_resource:
> > > find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size
> > > | wc -l
> > > 82
> > > I know, that was dumb, Coccinelle would certainly do better job.
> > > And from a quick look a lot of
> > > if (!res) {
> > > 	print error
> > > 	return -EINVAL;
> > > }
> > > code blocks could be deleted (and many cases where check for NULL
> > > resource
> > > is missing fixed).
> > > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

--- a/lib/devres.c
+++ b/lib/devres.c
@@ -22,6 +22,8 @@  static int devm_ioremap_match(struct device *dev, void *res, void *match_data)
  * @size: Size of map
  *
  * Managed ioremap().  Map is automatically unmapped on driver detach.
+ *
+ * When possible, use devm_ioremap_resource() instead.
  */
 void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
 			   resource_size_t size)