[PATCH v3 0/4] Fix null pointer deference when calling of_platform_depopulate

[PATCH v3 0/4] Fix null pointer deference when calling of_platform_depopulate

[Ricardo Ribalda Delgado]

of_platform_depopulate can lead to a kernel error when calling release_resource()

of_platform_depopulate()
      of_platform_device_destroy()
        platform_device_unregister(platform_device *pdev)
	  platform_device_del(platform_device *pdev)
	    for (i = 0; i < pdev->num_resources; i++)
	    	      release_resource()

The reason is that it is trying to release a resource that was not allocated
via insert_resource

of_platform_populate()
      ...
      	of_device_alloc()
	  pdev = platform_device_alloc()
	  # set pdev->resource, similar to platform_device_add_resources()
	  of_device_add(platform_device *pdev)
	    # similar to platform_device_add(), but note there's no
	    # insert_resource() in this path
	    device_add(&pdev->dev)

This set of patches modifies release_resource to check for
resource->parent==NULL and throw a warning if there is an error.

base/platform has been fixed for an hypothetical condition where parent is
set but the platform is neither MEM or IO.

Then platform_device_alloc has been modified so it supports of and amba 
devices.

Finally of_device_add has been modified to use platform_device_add().

v1: https://lkml.org/lkml/2015/4/20/435

v2: https://lkml.org/lkml/2015/4/21/99
    https://lkml.org/lkml/2015/4/21/100

Ricardo Ribalda Delgado (4):
  kernel/resource: Invalid memory access in __release_resource
  base/platform: Only insert MEM and IO resources
  base/platform: Continue on insert_resource() error
  of/platform: Use platform_device interface

 drivers/base/platform.c | 20 ++++++++++++--------
 drivers/of/platform.c   |  3 ++-
 kernel/resource.c       |  3 +++
 3 files changed, 17 insertions(+), 9 deletions(-)

-- 
2.1.4

[PATCH v3 1/4] kernel/resource: Invalid memory access in __release_resource

[Ricardo Ribalda Delgado]

When a resource is initialized via of_platform_populate.
resource->parent is initialized to NULL via kzalloc.
(of_platform_populate->of_device_alloc->of_address_to_resource)

If of_platform_depopulate is called later, resource->parent is
accessed (Offset 0x30 of address 0), causing a kernel error.

This patch evaluates resouce->parent before accessing it. If it
is not initialized, -EACCESS is returned.

Also a WARN is thrown, so the developer can have a hint about what
needs to be fixed.

Fixes:
BUG: unable to handle kernel NULL pointer deference at 0000000000000030
IP: release_resource+0x26/0x90
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 kernel/resource.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index 90552aa..b7b270f 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -237,6 +237,9 @@ static int __release_resource(struct resource *old)
 {
 	struct resource *tmp, **p;
 
+	if (WARN_ON(!old->parent))
+		return -EINVAL;
+
 	p = &old->parent->child;
 	for (;;) {
 		tmp = *p;
-- 
2.1.4

Re: [PATCH v3 1/4] kernel/resource: Invalid memory access in __release_resource

[Bjorn Helgaas]

On Wed, Apr 22, 2015 at 06:14:18PM +0200, Ricardo Ribalda Delgado wrote:
> When a resource is initialized via of_platform_populate.
> resource->parent is initialized to NULL via kzalloc.
> (of_platform_populate->of_device_alloc->of_address_to_resource)
> 
> If of_platform_depopulate is called later, resource->parent is
> accessed (Offset 0x30 of address 0), causing a kernel error.
> 
> This patch evaluates resouce->parent before accessing it. If it
> is not initialized, -EACCESS is returned.
> 
> Also a WARN is thrown, so the developer can have a hint about what
> needs to be fixed.
> 
> Fixes:
> BUG: unable to handle kernel NULL pointer deference at 0000000000000030
> IP: release_resource+0x26/0x90
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  kernel/resource.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 90552aa..b7b270f 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -237,6 +237,9 @@ static int __release_resource(struct resource *old)
>  {
>  	struct resource *tmp, **p;
>  
> +	if (WARN_ON(!old->parent))
> +		return -EINVAL;

I'm not really a fan of this.  The NULL pointer oops is a very good clue
all by itself, and it doesn't require any extra code here.

>  	p = &old->parent->child;
>  	for (;;) {
>  		tmp = *p;
> -- 
> 2.1.4
> 

Re: [PATCH v3 1/4] kernel/resource: Invalid memory access in __release_resource

[Ricardo Ribalda Delgado]

Hi Bjorn

On Wed, Apr 22, 2015 at 6:47 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

>
> I'm not really a fan of this.  The NULL pointer oops is a very good clue
> all by itself, and it doesn't require any extra code here.

It is a pointer to 0x30.

For some reason my platform can handle a couple of oops, but if I get
a fair amount of them in a short period of time, the whole thing
crashes and debugging gets complicated. (no access to dmesg or remote
debugging).

Therefore this particular bug gave me a bad time. Now that we have
located the root of the problem and solved the problem from the root,
this particular error will never be hit. I just wanted to save
somebody else's time with this patch.

All that said, if the other patches are applied, my platform works
completely fine. So it is completely your call to accept this or not
:)

I will resend the patchset without this patch

Thanks!
-- 
Ricardo Ribalda

[PATCH v3 2/4] base/platform: Only insert MEM and IO resources

[Ricardo Ribalda Delgado]

platform_device_del only checks the type of the resource in order to
call release_resource.

On the other hand, platform_device_add calls insert_resource for any
resource that has a parent.

Make both code branches balanced.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/base/platform.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index ebf034b..6028681 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -341,19 +341,23 @@ int platform_device_add(struct platform_device *pdev)
 
 	for (i = 0; i < pdev->num_resources; i++) {
 		struct resource *p, *r = &pdev->resource[i];
+		unsigned long type = resource_type(r);
 
 		if (r->name == NULL)
 			r->name = dev_name(&pdev->dev);
 
+		if (!(type == IORESOURCE_MEM || type == IORESOURCE_IO))
+			continue;
+
 		p = r->parent;
 		if (!p) {
-			if (resource_type(r) == IORESOURCE_MEM)
+			if (type == IORESOURCE_MEM)
 				p = &iomem_resource;
-			else if (resource_type(r) == IORESOURCE_IO)
+			else if (type == IORESOURCE_IO)
 				p = &ioport_resource;
 		}
 
-		if (p && insert_resource(p, r)) {
+		if (insert_resource(p, r)) {
 			dev_err(&pdev->dev, "failed to claim resource %d\n", i);
 			ret = -EBUSY;
 			goto failed;
-- 
2.1.4

[PATCH v3 3/4] base/platform: Continue on insert_resource() error

[Ricardo Ribalda Delgado]

insert_resource() can fail when the resource added  overlaps
(partially or fully) with another.

Device tree and AMBA devices may contain resources that overlap, so they
could not call platform_device_add (Revert "of: use platform_device_add"
02bbde7849e68e193cefaa1885fe0df0f03c9fcd )

On the other hand, device trees are released using
platform_device_unregister(). This function calls platform_device_del(),
which calls release_resource(), that crashes when the resource has not
been added with with insert_resource. This was not an issue when the
device tree could not be modified online, but this is not the case
anymore.

This patch let the flow continue when there is an insert error, after
notifying the user with a dev_err(). r->parent is set to NULL, so the
device_del knows that the resource was not added, and therefore it
should not be released.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/base/platform.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 6028681..8cce2a3 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -359,8 +359,7 @@ int platform_device_add(struct platform_device *pdev)
 
 		if (insert_resource(p, r)) {
 			dev_err(&pdev->dev, "failed to claim resource %d\n", i);
-			ret = -EBUSY;
-			goto failed;
+			r->parent = NULL;
 		}
 	}
 
@@ -371,7 +370,6 @@ int platform_device_add(struct platform_device *pdev)
 	if (ret == 0)
 		return ret;
 
- failed:
 	if (pdev->id_auto) {
 		ida_simple_remove(&platform_devid_ida, pdev->id);
 		pdev->id = PLATFORM_DEVID_AUTO;
@@ -381,7 +379,8 @@ int platform_device_add(struct platform_device *pdev)
 		struct resource *r = &pdev->resource[i];
 		unsigned long type = resource_type(r);
 
-		if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
+		if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
+				r->parent)
 			release_resource(r);
 	}
 
@@ -414,7 +413,8 @@ void platform_device_del(struct platform_device *pdev)
 			struct resource *r = &pdev->resource[i];
 			unsigned long type = resource_type(r);
 
-			if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
+			if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
+					r->parent)
 				release_resource(r);
 		}
 	}
-- 
2.1.4

Re: [PATCH v3 3/4] base/platform: Continue on insert_resource() error

[Bjorn Helgaas]

On Wed, Apr 22, 2015 at 06:14:20PM +0200, Ricardo Ribalda Delgado wrote:
> insert_resource() can fail when the resource added  overlaps
> (partially or fully) with another.
> 
> Device tree and AMBA devices may contain resources that overlap, so they
> could not call platform_device_add (Revert "of: use platform_device_add"
> 02bbde7849e68e193cefaa1885fe0df0f03c9fcd )

Usual style for referencing a commit is "(see 02bbde7849e6 ('Revert "of:
use platform_device_add"'))".

> On the other hand, device trees are released using
> platform_device_unregister(). This function calls platform_device_del(),
> which calls release_resource(), that crashes when the resource has not
> been added with with insert_resource. This was not an issue when the
> device tree could not be modified online, but this is not the case
> anymore.
> 
> This patch let the flow continue when there is an insert error, after
> notifying the user with a dev_err(). r->parent is set to NULL, so the
> device_del knows that the resource was not added, and therefore it
> should not be released.

I think you meant platform_device_del()?  I don't think device_del() does
anything with resources.

> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/base/platform.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 6028681..8cce2a3 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -359,8 +359,7 @@ int platform_device_add(struct platform_device *pdev)
>  
>  		if (insert_resource(p, r)) {
>  			dev_err(&pdev->dev, "failed to claim resource %d\n", i);

Sounds like we should expect to see this message more in the future, after
you change of_platform_device_create_pdata() use platform_device_add().
You might want to use insert_resource_conflict() here so you can include
information about *why* we failed to claim the resource.  And it would be
nice to use %pR for both resources.

> -			ret = -EBUSY;
> -			goto failed;
> +			r->parent = NULL;
>  		}
>  	}
>  
> @@ -371,7 +370,6 @@ int platform_device_add(struct platform_device *pdev)
>  	if (ret == 0)
>  		return ret;
>  
> - failed:

Might be nice to keep a comment here as a clue that the rest of the
function is the failure path.

It's a minor style thing, but I would also remove the "err_out" label and
return "ret" directly above rather than branching to "err_out".

>  	if (pdev->id_auto) {
>  		ida_simple_remove(&platform_devid_ida, pdev->id);
>  		pdev->id = PLATFORM_DEVID_AUTO;
> @@ -381,7 +379,8 @@ int platform_device_add(struct platform_device *pdev)
>  		struct resource *r = &pdev->resource[i];
>  		unsigned long type = resource_type(r);
>  
> -		if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
> +		if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
> +				r->parent)
>  			release_resource(r);
>  	}
>  
> @@ -414,7 +413,8 @@ void platform_device_del(struct platform_device *pdev)
>  			struct resource *r = &pdev->resource[i];
>  			unsigned long type = resource_type(r);
>  
> -			if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
> +			if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
> +					r->parent)
>  				release_resource(r);
>  		}
>  	}
> -- 
> 2.1.4
> 

Re: [PATCH v3 3/4] base/platform: Continue on insert_resource() error

[Ricardo Ribalda Delgado]

Hi Bjorn:

On Wed, Apr 22, 2015 at 6:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> Usual style for referencing a commit is "(see 02bbde7849e6 ('Revert "of:
> use platform_device_add"'))".

Do you make that reference manually or there is a magic git command
for printing it in that style?

>

> Sounds like we should expect to see this message more in the future, after
> you change of_platform_device_create_pdata() use platform_device_add().
> You might want to use insert_resource_conflict() here so you can include
> information about *why* we failed to claim the resource.  And it would be
> nice to use %pR for both resources.
>
.....

>>
>> - failed:
>
> Might be nice to keep a comment here as a clue that the rest of the
> function is the failure path.


Will prepare a patch with the changes and resend.


Thanks!

-- 
Ricardo Ribalda

Re: [PATCH v3 3/4] base/platform: Continue on insert_resource() error

[Bjorn Helgaas]

On Thu, Apr 23, 2015 at 09:55:13AM +0200, Ricardo Ribalda Delgado wrote:
> Hi Bjorn:
> 
> On Wed, Apr 22, 2015 at 6:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 
> > Usual style for referencing a commit is "(see 02bbde7849e6 ('Revert "of:
> > use platform_device_add"'))".
> 
> Do you make that reference manually or there is a magic git command
> for printing it in that style?

I used to do it mostly by hand, but thanks to your prompting, I fiddled
around and came up with this alias:

gsr is aliased to `git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"'

Bjorn

Re: [PATCH v3 3/4] base/platform: Continue on insert_resource() error

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On Thu, Apr 23, 2015 at 08:26:37AM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 23, 2015 at 09:55:13AM +0200, Ricardo Ribalda Delgado wrote:
> > Hi Bjorn:
> > 
> > On Wed, Apr 22, 2015 at 6:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > 
> > > Usual style for referencing a commit is "(see 02bbde7849e6 ('Revert "of:
> > > use platform_device_add"'))".
> > 
> > Do you make that reference manually or there is a magic git command
> > for printing it in that style?
> 
> I used to do it mostly by hand, but thanks to your prompting, I fiddled
> around and came up with this alias:
> 
> gsr is aliased to `git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"'

There's also git log --format=fixes, though that includes the "Fixes: "
prefix. That works with git show as well.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v3 4/4] of/platform: Use platform_device interface

[Ricardo Ribalda Delgado]

of_platform_device_create_pdata() was using of_device_add() to create
the devices, but of_platform_device_destroy was using
of_platform_device_destroy().

of_device_add(), do not call insert_resource(), which initializes the
parent field of the resource structure, needed by release_resource(),
called by of_platform_device_destroy().

This leads to a NULL pointer deference.

This patch, replaces of_device_add() with platform_device_data().

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/of/platform.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index a01f57c..f011f57 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -183,8 +183,9 @@ static struct platform_device *of_platform_device_create_pdata(
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
 	of_dma_configure(&dev->dev, dev->dev.of_node);
+	dev->name = dev_name(&dev->dev);
 
-	if (of_device_add(dev) != 0) {
+	if (platform_device_add(dev) != 0) {
 		of_dma_deconfigure(&dev->dev);
 		platform_device_put(dev);
 		goto err_clear_flag;
-- 
2.1.4

Re: [PATCH v3 4/4] of/platform: Use platform_device interface

[Rob Herring]

On Wed, Apr 22, 2015 at 11:14 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> of_platform_device_create_pdata() was using of_device_add() to create
> the devices, but of_platform_device_destroy was using
> of_platform_device_destroy().
>
> of_device_add(), do not call insert_resource(), which initializes the
> parent field of the resource structure, needed by release_resource(),
> called by of_platform_device_destroy().
>
> This leads to a NULL pointer deference.
>
> This patch, replaces of_device_add() with platform_device_data().

This doesn't match the change.

>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/of/platform.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index a01f57c..f011f57 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -183,8 +183,9 @@ static struct platform_device *of_platform_device_create_pdata(
>         dev->dev.bus = &platform_bus_type;
>         dev->dev.platform_data = platform_data;
>         of_dma_configure(&dev->dev, dev->dev.of_node);
> +       dev->name = dev_name(&dev->dev);
>
> -       if (of_device_add(dev) != 0) {
> +       if (platform_device_add(dev) != 0) {

Can't we now remove of_device_add?

>                 of_dma_deconfigure(&dev->dev);
>                 platform_device_put(dev);
>                 goto err_clear_flag;
> --
> 2.1.4
>

Re: [PATCH v3 4/4] of/platform: Use platform_device interface

[Ricardo Ribalda Delgado]

Hello Rob

On Wed, Apr 22, 2015 at 6:25 PM, Rob Herring <robherring2@gmail.com> wrote:

>> This patch, replaces of_device_add() with platform_device_data().
>
> This doesn't match the change.

Thanks for catching it up. Will fix it and resend

>> -       if (of_device_add(dev) != 0) {
>> +       if (platform_device_add(dev) != 0) {
>
> Can't we now remove of_device_add?

Now it is only used by ibmebus


ricardo@neopili:~/linux-upstream$ git grep of_device_add
arch/powerpc/kernel/ibmebus.c:  ret = of_device_add(dev);
drivers/of/device.c:int of_device_add(struct platform_device *ofdev)
drivers/of/device.c:    return of_device_add(pdev);
include/linux/of_device.h:extern int of_device_add(struct
platform_device *pdev);

I think it can also use platform_device_add. I will prepare a patch to
finally remove of_device_add()

Thanks!




-- 
Ricardo Ribalda

Re: [PATCH v3 4/4] of/platform: Use platform_device interface

[Ricardo Ribalda Delgado]

Hello Rob

On Thu, Apr 23, 2015 at 9:28 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
>
> I think it can also use platform_device_add. I will prepare a patch to
> finally remove of_device_add()

I will post right away an updated version of this patchset, and then I
will prepare another one to remove of_device_add() completely. I
already have two patches, but I have to figure out a proper way to
test it.

Thanks!


-- 
Ricardo Ribalda

Re: [PATCH v3.1 4/4] of/platform: Use platform_device interface

[Greg Kroah-Hartman]

On Fri, May 15, 2015 at 01:52:10PM +0200, Ricardo Ribalda Delgado wrote:
> of_platform_device_create_pdata() was using of_device_add() to create
> the devices, but of_platform_device_destroy was using
> platform_device_unregister() to free them.
> 
> of_device_add(), do not call insert_resource(), which initializes the
> parent field of the resource structure, needed by release_resource(),
> called by of_platform_device_destroy(). This leads to a NULL pointer
> deference.
> 
> Instead of fixing the NULL pointer deference, what could hide other bugs,
> this patch, replaces of_device_add() with platform_device_data().
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> 
> v3.1 Fix comments by Rob Herring, thanks!

3.1?

Please resend the whole series, this is a mess, I can't find where this
goes at all...

greg k-h

Re: [PATCH v3.1 4/4] of/platform: Use platform_device interface

[Ricardo Ribalda Delgado]

Hello Greg

Sorry for the mess. I did not want to spam the mailing list too much.

Repacking and resending. Thanks!

On Sun, May 24, 2015 at 9:29 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, May 15, 2015 at 01:52:10PM +0200, Ricardo Ribalda Delgado wrote:
>> of_platform_device_create_pdata() was using of_device_add() to create
>> the devices, but of_platform_device_destroy was using
>> platform_device_unregister() to free them.
>>
>> of_device_add(), do not call insert_resource(), which initializes the
>> parent field of the resource structure, needed by release_resource(),
>> called by of_platform_device_destroy(). This leads to a NULL pointer
>> deference.
>>
>> Instead of fixing the NULL pointer deference, what could hide other bugs,
>> this patch, replaces of_device_add() with platform_device_data().
>>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>
>> v3.1 Fix comments by Rob Herring, thanks!
>
> 3.1?
>
> Please resend the whole series, this is a mess, I can't find where this
> goes at all...
>
> greg k-h



-- 
Ricardo Ribalda