[PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Wei Yongjun]

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);

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;

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Bjorn Helgaas]

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);

This pattern of:

  platform_get_resource_byname
  devm_ioremap

is extremely common and could perhaps be factored out.  There are
already a few private versions, e.g., request_and_map(),
msm_ioremap(), ssi_get_iomem().

request_and_map() also includes devm_request_mem_region(), which
probably *should* be included but usually seems to be omitted.

> 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;
> 

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Ladislav Michl]

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).

	ladis

> 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

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Bjorn Helgaas]

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)

> > 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

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Ladislav Michl]

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

RE: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[weiyongjun (A)]

> 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).
> 

RE: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Julia Lawall]

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
>

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Ladislav Michl]

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).
> > 

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Ladislav Michl]

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

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Ladislav Michl]

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:

That seems to spot another a bit more serious problem (given how late
release cycle is now).

Both devm_ioremap() and devm_ioremap_resource() shares the same release
function: devm_ioremap_release(). However this function is not aware of
memory region previously requested by devm_request_mem_region() called
from devm_ioremap_resource().

Bellow is just a quick hack, even untested as looking at devm_ioremap,
devm_ioremap_wc and devm_ioremap_wc, there is some room for optimization.

diff --git a/lib/devres.c b/lib/devres.c
index 5f2aedd58bc5..6315b07a608f 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -10,6 +10,15 @@ void devm_ioremap_release(struct device *dev, void *res)
 	iounmap(*(void __iomem **)res);
 }
 
+void devm_ioremap_release_region(struct device *dev, void *res)
+{
+	resource_size_t offset = ((struct resource *)res)->start;
+	resource_size_t size = resource_size((struct resource *)res);
+
+	iounmap(*(void __iomem **)res);
+	devm_release_mem_region(dev, offset, size);
+}
+
 static int devm_ioremap_match(struct device *dev, void *res, void *match_data)
 {
 	return *(void **)res = match_data;
@@ -136,7 +145,7 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res)
 {
 	resource_size_t size;
 	const char *name;
-	void __iomem *dest_ptr;
+	void __iomem *addr, **ptr;
 
 	BUG_ON(!dev);
 
@@ -153,14 +162,25 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res)
 		return IOMEM_ERR_PTR(-EBUSY);
 	}
 
-	dest_ptr = devm_ioremap(dev, res->start, size);
-	if (!dest_ptr) {
+	ptr = devres_alloc(devm_ioremap_release_region, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr) {
+		dev_err(dev, "malloc failed for resource %pR\n", res);
+		devm_release_mem_region(dev, res->start, size);
+		return IOMEM_ERR_PTR(-ENOMEM);
+	}
+
+	addr = ioremap(res->start, size);
+	if (addr) {
+		*ptr = addr;
+		devres_add(dev, ptr);
+	} else {
 		dev_err(dev, "ioremap failed for resource %pR\n", res);
 		devm_release_mem_region(dev, res->start, size);
-		dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
+		devres_free(ptr);
+		addr = IOMEM_ERR_PTR(-ENOMEM);
 	}
 
-	return dest_ptr;
+	return addr;
 }
 EXPORT_SYMBOL(devm_ioremap_resource);
 

> --- 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)
> 
> > > 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

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Ladislav Michl]

On Fri, Jan 19, 2018 at 10:58:57AM +0100, Ladislav Michl wrote:
> On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote:
> > That's probably a better idea.  Maybe we should add a comment like this
> > to help avoid this in the future:
> 
> That seems to spot another a bit more serious problem (given how late
> release cycle is now).
> 
> Both devm_ioremap() and devm_ioremap_resource() shares the same release
> function: devm_ioremap_release(). However this function is not aware of
> memory region previously requested by devm_request_mem_region() called
> from devm_ioremap_resource().
> 
> Bellow is just a quick hack, even untested as looking at devm_ioremap,
> devm_ioremap_wc and devm_ioremap_wc, there is some room for optimization.

Okay, forget it, above analysis is not correct, however there is a bug (and
also in PCI version). To show it, let's make following modification:

diff --git a/lib/devres.c b/lib/devres.c
index e9aad136f667..193e540eab23 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -153,6 +153,10 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res)
 		return IOMEM_ERR_PTR(-EBUSY);
 	}
 
+	if (res->start = 0x4809c000 || res->start = 0x480b4000 || res->start = 0x480ad000) {
+		dev_info(dev, "Setting size to madness\n");
+		size = 1000000000;
+	}
 	dest_ptr = devm_ioremap(dev, res->start, size);
 	if (!dest_ptr) {
 		dev_err(dev, "ioremap failed for resource %pR\n", res);

Above patch will set insane resource size for omap_hsmmc driver which is
using devm_ioremap_resource() and triggers following error:

vmap allocation for size 1000005632 failed: use vmalloc=<size> to increase size
omap_hsmmc 4809c000.mmc: ioremap failed for resource [mem 0x4809c000-0x4809c1ff]
Trying to free nonexistent resource <000000004809c000-0000000083a489ff>
------------[ cut here ]------------
WARNING: CPU: 0 PID: 92 at kernel/resource.c:1477 __devm_release_region+0x44/0x58
Modules linked in: omap_aes_driver(+) omap_sham(+) crypto_engine omap_crypto phy_twl4030_usb omap2430(+) omap_hsmmc(+) musb_hdrc omap_mailbox ohci_platform(+) snd_soc_twl4030 ohci_hcd ehci_omap td
CPU: 0 PID: 92 Comm: systemd-udevd Not tainted 4.15.0-rc8-next-20180118 #42
Hardware name: Generic OMAP36xx (Flattened Device Tree)
[<c010d450>] (unwind_backtrace) from [<c010b510>] (show_stack+0x10/0x14)
[<c010b510>] (show_stack) from [<c0129648>] (__warn+0xd4/0xec)
[<c0129648>] (__warn) from [<c0129720>] (warn_slowpath_null+0x38/0x44)
[<c0129720>] (warn_slowpath_null) from [<c012cd4c>] (__devm_release_region+0x44/0x58)
[<c012cd4c>] (__devm_release_region) from [<c02b9904>] (devm_ioremap_resource+0x118/0x140)
[<c02b9904>] (devm_ioremap_resource) from [<bf0f9f0c>] (omap_hsmmc_probe+0x15c/0x960 [omap_hsmmc])
[<bf0f9f0c>] (omap_hsmmc_probe [omap_hsmmc]) from [<c0339204>] (platform_drv_probe+0x50/0x9c)
[<c0339204>] (platform_drv_probe) from [<c0337c04>] (driver_probe_device+0x330/0x478)
[<c0337c04>] (driver_probe_device) from [<c0337dec>] (__driver_attach+0xa0/0x104)
[<c0337dec>] (__driver_attach) from [<c0335fe8>] (bus_for_each_dev+0x54/0x78)
[<c0335fe8>] (bus_for_each_dev) from [<c0336fbc>] (bus_add_driver+0x1b4/0x22c)
[<c0336fbc>] (bus_add_driver) from [<c03384bc>] (driver_register+0xa0/0xe0)
[<c03384bc>] (driver_register) from [<c0102620>] (do_one_initcall+0x124/0x14c)
[<c0102620>] (do_one_initcall) from [<c016d300>] (do_init_module+0x54/0x1c0)
[<c016d300>] (do_init_module) from [<c016ca48>] (load_module+0x1e90/0x1fb0)
[<c016ca48>] (load_module) from [<c016cd80>] (SyS_finit_module+0xb4/0xc4)
[<c016cd80>] (SyS_finit_module) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
Exception stack(0xdde2bfa8 to 0xdde2bff0)
bfa0:                   004f23b0 beb7cdc4 00000007 b6f7e0d8 00000000 004f22f8
bfc0: 004f23b0 beb7cdc4 beb7cdbc 0000017b 004faec8 00000000 00000000 004fd1d0
bfe0: beb7cca8 beb7cc98 b6f77cc5 b6edfba2
---[ end trace b8768b734ce0c288 ]---
omap_hsmmc: probe of 4809c000.mmc failed with error -12

Please note that "Trying to free nonexistent resource" caused by calling
devm_release_mem_region() twice.

Fixes will be sent separately.

Best regards,
	ladis

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Ladislav Michl]

On Fri, Jan 19, 2018 at 06:06:57PM +0100, Ladislav Michl wrote:
> On Fri, Jan 19, 2018 at 10:58:57AM +0100, Ladislav Michl wrote:
> > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote:
> > > That's probably a better idea.  Maybe we should add a comment like this
> > > to help avoid this in the future:
> > 
> > That seems to spot another a bit more serious problem (given how late
> > release cycle is now).
> > 
> > Both devm_ioremap() and devm_ioremap_resource() shares the same release
> > function: devm_ioremap_release(). However this function is not aware of
> > memory region previously requested by devm_request_mem_region() called
> > from devm_ioremap_resource().
> > 
> > Bellow is just a quick hack, even untested as looking at devm_ioremap,
> > devm_ioremap_wc and devm_ioremap_wc, there is some room for optimization.
> 
> Okay, forget it, above analysis is not correct, however there is a bug (and
> also in PCI version). To show it, let's make following modification:

I will never ever work in single tree for two different boards without full
recompile (which should save time and caused opposite) as it makes debugging
pointless - there is no bug.

As a request forgiveness, please accept following draft as proposed solution
for $subj

Subject: [PATCH] PCI: dra7xx: Use devm_ioremap_resource()

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index 8bf7c2714db6..7f422ae258ac 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -409,14 +409,14 @@ 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");
-	pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res));
-	if (!pci->dbi_base)
-		return -ENOMEM;
+	pci->dbi_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pci->dbi_base))
+		return PTR_ERR(pci->dbi_base);
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics2");
-	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
-	if (!pci->dbi_base2)
-		return -ENOMEM;
+	pci->dbi_base2 = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pci->dbi_base2))
+		return PTR_ERR(pci->dbi_base2);
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
 	if (!res)

Re: [PATCH -next] PCI: dra7xx: Fix potential NULL dereference

[Lorenzo Pieralisi]

On Sat, Jan 20, 2018 at 01:16:45AM +0100, Ladislav Michl wrote:
> On Fri, Jan 19, 2018 at 06:06:57PM +0100, Ladislav Michl wrote:
> > On Fri, Jan 19, 2018 at 10:58:57AM +0100, Ladislav Michl wrote:
> > > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote:
> > > > That's probably a better idea.  Maybe we should add a comment like this
> > > > to help avoid this in the future:
> > > 
> > > That seems to spot another a bit more serious problem (given how late
> > > release cycle is now).
> > > 
> > > Both devm_ioremap() and devm_ioremap_resource() shares the same release
> > > function: devm_ioremap_release(). However this function is not aware of
> > > memory region previously requested by devm_request_mem_region() called
> > > from devm_ioremap_resource().
> > > 
> > > Bellow is just a quick hack, even untested as looking at devm_ioremap,
> > > devm_ioremap_wc and devm_ioremap_wc, there is some room for optimization.
> > 
> > Okay, forget it, above analysis is not correct, however there is a bug (and
> > also in PCI version). To show it, let's make following modification:
> 
> I will never ever work in single tree for two different boards without full
> recompile (which should save time and caused opposite) as it makes debugging
> pointless - there is no bug.
> 
> As a request forgiveness, please accept following draft as proposed solution
> for $subj

Wei, Ladislav,

getting back to this old thread, I would mark it as "changes requested"
and expect someone to post a follow-up patch, I do not think this is
a solved problem.

Lorenzo

> Subject: [PATCH] PCI: dra7xx: Use devm_ioremap_resource()
> 
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index 8bf7c2714db6..7f422ae258ac 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -409,14 +409,14 @@ 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");
> -	pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res));
> -	if (!pci->dbi_base)
> -		return -ENOMEM;
> +	pci->dbi_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pci->dbi_base))
> +		return PTR_ERR(pci->dbi_base);
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics2");
> -	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
> -	if (!pci->dbi_base2)
> -		return -ENOMEM;
> +	pci->dbi_base2 = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pci->dbi_base2))
> +		return PTR_ERR(pci->dbi_base2);
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>  	if (!res)