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

[PATCH v5 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()

The reason is that it is trying to release a resource that was not allocated
via insert_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()

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)

On this patchset:

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: From: Bjorn Helgaas <bhelgaas@google.com>
     -Fix caller, not callee

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

v3: From: Rob Herring <robherring2@gmail.com>
      - Modify plaform_device_alloc to support of and ambda devices

    https://lkml.org/lkml/2015/4/22/369
    https://lkml.org/lkml/2015/4/22/370
    https://lkml.org/lkml/2015/4/22/371
    https://lkml.org/lkml/2015/4/22/374
    https://lkml.org/lkml/2015/4/22/373

v4: From: Bjorn Helgaas <bhelgaas@google.com>
     -Remove WARN() patch
     -Show conflicting resources
     -Code Style
     -Fix descriptions

    From: Rob Herring <robherring2@gmail.com>
      -Fix descriptions

    https://lkml.org/lkml/2015/4/23/254
    https://lkml.org/lkml/2015/4/23/258
    https://lkml.org/lkml/2015/4/23/257
    https://lkml.org/lkml/2015/4/23/256
    https://lkml.org/lkml/2015/4/23/255

v5: From: Rob Herring <robherring2@gmail.com>
       -Fix descriptions

Ricardo Ribalda Delgado (4):
  base/platform: Only insert MEM and IO resources
  base/platform: Continue on insert_resource() error
  of/platform: Use platform_device interface
  base/platform: Remove code duplication

 drivers/base/platform.c | 84 ++++++++++++++++++++++++-------------------------
 drivers/of/platform.c   |  3 +-
 2 files changed, 43 insertions(+), 44 deletions(-)

-- 
2.1.4

[PATCH v5 1/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 063f0ab15259..46a56f694cec 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 v5 2/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 (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
platform_device_del() knows that the resource was not added, and
therefore it should not be released.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/base/platform.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 46a56f694cec..5a29387e5ff6 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -332,7 +332,7 @@ int platform_device_add(struct platform_device *pdev)
 		 */
 		ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL);
 		if (ret < 0)
-			goto err_out;
+			return ret;
 		pdev->id = ret;
 		pdev->id_auto = true;
 		dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id);
@@ -340,7 +340,7 @@ int platform_device_add(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < pdev->num_resources; i++) {
-		struct resource *p, *r = &pdev->resource[i];
+		struct resource *conflict, *p, *r = &pdev->resource[i];
 		unsigned long type = resource_type(r);
 
 		if (r->name == NULL)
@@ -357,11 +357,14 @@ int platform_device_add(struct platform_device *pdev)
 				p = &ioport_resource;
 		}
 
-		if (insert_resource(p, r)) {
-			dev_err(&pdev->dev, "failed to claim resource %d\n", i);
-			ret = -EBUSY;
-			goto failed;
-		}
+		conflict = insert_resource_conflict(p, r);
+		if (!conflict)
+			continue;
+
+		dev_err(&pdev->dev,
+			"ignoring resource %pR (conflicts with %s %pR)\n",
+			r, conflict->name, conflict);
+		p->parent = NULL;
 	}
 
 	pr_debug("Registering platform device '%s'. Parent at %s\n",
@@ -371,7 +374,7 @@ int platform_device_add(struct platform_device *pdev)
 	if (ret == 0)
 		return ret;
 
- failed:
+	/* Failure path */
 	if (pdev->id_auto) {
 		ida_simple_remove(&platform_devid_ida, pdev->id);
 		pdev->id = PLATFORM_DEVID_AUTO;
@@ -381,11 +384,11 @@ 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);
 	}
 
- err_out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(platform_device_add);
@@ -414,7 +417,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 v5 2/4] base/platform: Continue on insert_resource() error

[Grant Likely]

On Tue, 26 May 2015 09:31:24 +0200
, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
 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 (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
> platform_device_del() knows that the resource was not added, and
> therefore it should not be released.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/base/platform.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 46a56f694cec..5a29387e5ff6 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -332,7 +332,7 @@ int platform_device_add(struct platform_device *pdev)
>  		 */
>  		ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL);
>  		if (ret < 0)
> -			goto err_out;
> +			return ret;
>  		pdev->id = ret;
>  		pdev->id_auto = true;
>  		dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id);
> @@ -340,7 +340,7 @@ int platform_device_add(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < pdev->num_resources; i++) {
> -		struct resource *p, *r = &pdev->resource[i];
> +		struct resource *conflict, *p, *r = &pdev->resource[i];
>  		unsigned long type = resource_type(r);
>  
>  		if (r->name == NULL)
> @@ -357,11 +357,14 @@ int platform_device_add(struct platform_device *pdev)
>  				p = &ioport_resource;
>  		}
>  
> -		if (insert_resource(p, r)) {
> -			dev_err(&pdev->dev, "failed to claim resource %d\n", i);
> -			ret = -EBUSY;
> -			goto failed;
> -		}
> +		conflict = insert_resource_conflict(p, r);
> +		if (!conflict)
> +			continue;
> +
> +		dev_err(&pdev->dev,
> +			"ignoring resource %pR (conflicts with %s %pR)\n",
> +			r, conflict->name, conflict);
> +		p->parent = NULL;

I'm pretty sure this is going to break some platforms. I described it in
my earlier email today, but I'll summarize here too since this is the
latest patch set.

Making this change allows the registration of devices to continue, but
it will still break device drivers that do a request_resource() on a
region that another device managed to claim with insert_resource(). The
only way around this is to not do insert_resource() at all, or to remove
the request_resource() from all drivers (not feasible).

I think we have to deal with it by making resource insertion optional.
I'd like to make the default be to do the insertion, and be able to
blacklist platforms that have issues.

g.

Re: [PATCH v5 2/4] base/platform: Continue on insert_resource() error

[Ricardo Ribalda Delgado]

Hello Grant

On Thu, Jun 4, 2015 at 9:54 AM, Grant Likely <grant.likely@linaro.org> wrote:

>
> I'm pretty sure this is going to break some platforms. I described it in
> my earlier email today, but I'll summarize here too since this is the
> latest patch set.


The version that is in stable is also broken. Unloading the device
tree crashes the device completely.

>
> Making this change allows the registration of devices to continue, but
> it will still break device drivers that do a request_resource() on a
> region that another device managed to claim with insert_resource(). The
> only way around this is to not do insert_resource() at all, or to remove
> the request_resource() from all drivers (not feasible).

If we do not do insert_resource(), we will have the crash on
release_resource() and a lot! of code duplication.

>
> I think we have to deal with it by making resource insertion optional.
> I'd like to make the default be to do the insertion, and be able to
> blacklist platforms that have issues.

What about, making the request_resource a little bit more clever.
Something like:

If the resouce is not taken
     return ok

if the resource is taken:
  If the requester or the current owner of the resource are device tree devices
               show a warning and continue.
  else
                return error


Wouldn't this fix the issue and guide the developers towards a proper
fix for their platform, instead of just encourage them to blacklist
their platform?


Thanks!


>
> g.
>



-- 
Ricardo Ribalda

Re: [PATCH v5 2/4] base/platform: Continue on insert_resource() error

[Rob Herring]

On Thu, Jun 4, 2015 at 2:54 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Tue, 26 May 2015 09:31:24 +0200
> , Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>  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 (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
>> platform_device_del() knows that the resource was not added, and
>> therefore it should not be released.
>>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>>  drivers/base/platform.c | 26 +++++++++++++++-----------
>>  1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index 46a56f694cec..5a29387e5ff6 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -332,7 +332,7 @@ int platform_device_add(struct platform_device *pdev)
>>                */
>>               ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL);
>>               if (ret < 0)
>> -                     goto err_out;
>> +                     return ret;
>>               pdev->id = ret;
>>               pdev->id_auto = true;
>>               dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id);
>> @@ -340,7 +340,7 @@ int platform_device_add(struct platform_device *pdev)
>>       }
>>
>>       for (i = 0; i < pdev->num_resources; i++) {
>> -             struct resource *p, *r = &pdev->resource[i];
>> +             struct resource *conflict, *p, *r = &pdev->resource[i];
>>               unsigned long type = resource_type(r);
>>
>>               if (r->name == NULL)
>> @@ -357,11 +357,14 @@ int platform_device_add(struct platform_device *pdev)
>>                               p = &ioport_resource;
>>               }
>>
>> -             if (insert_resource(p, r)) {
>> -                     dev_err(&pdev->dev, "failed to claim resource %d\n", i);
>> -                     ret = -EBUSY;
>> -                     goto failed;
>> -             }
>> +             conflict = insert_resource_conflict(p, r);
>> +             if (!conflict)
>> +                     continue;
>> +
>> +             dev_err(&pdev->dev,
>> +                     "ignoring resource %pR (conflicts with %s %pR)\n",
>> +                     r, conflict->name, conflict);
>> +             p->parent = NULL;
>
> I'm pretty sure this is going to break some platforms. I described it in
> my earlier email today, but I'll summarize here too since this is the
> latest patch set.
>
> Making this change allows the registration of devices to continue, but
> it will still break device drivers that do a request_resource() on a
> region that another device managed to claim with insert_resource(). The
> only way around this is to not do insert_resource() at all, or to remove
> the request_resource() from all drivers (not feasible).

Do we have some clue as to which platforms have problems? I seem to
recall some i.MX platform.

> I think we have to deal with it by making resource insertion optional.
> I'd like to make the default be to do the insertion, and be able to
> blacklist platforms that have issues.

Yes, otherwise we'll never put this issue to bed.

Rob

[PATCH v5 3/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
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>
---

v5:  Fix comments by Rob Herring, thanks!

 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 ddf8e42c9367..236a8dfa83e6 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -184,8 +184,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

[PATCH v5 4/4] base/platform: Remove code duplication

[Ricardo Ribalda Delgado]

Failure path of platform_device_add was almost the same as
platform_device_del. Refactor same code in a function.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/base/platform.c | 60 +++++++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 35 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 5a29387e5ff6..cba8e0e83bfc 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -298,6 +298,25 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
 }
 EXPORT_SYMBOL_GPL(platform_device_add_data);
 
+static void platform_device_cleanout(struct platform_device *pdev, int n_res)
+{
+	int i;
+
+	if (pdev->id_auto) {
+		ida_simple_remove(&platform_devid_ida, pdev->id);
+		pdev->id = PLATFORM_DEVID_AUTO;
+	}
+
+	for (i = 0; i < n_res; i++) {
+		struct resource *r = &pdev->resource[i];
+		unsigned long type = resource_type(r);
+
+		if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
+				r->parent)
+			release_resource(r);
+	}
+}
+
 /**
  * platform_device_add - add a platform device to device hierarchy
  * @pdev: platform device we're adding
@@ -371,23 +390,8 @@ int platform_device_add(struct platform_device *pdev)
 		 dev_name(&pdev->dev), dev_name(pdev->dev.parent));
 
 	ret = device_add(&pdev->dev);
-	if (ret == 0)
-		return ret;
-
-	/* Failure path */
-	if (pdev->id_auto) {
-		ida_simple_remove(&platform_devid_ida, pdev->id);
-		pdev->id = PLATFORM_DEVID_AUTO;
-	}
-
-	while (--i >= 0) {
-		struct resource *r = &pdev->resource[i];
-		unsigned long type = resource_type(r);
-
-		if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
-				r->parent)
-			release_resource(r);
-	}
+	if (ret)
+		platform_device_cleanout(pdev, i);
 
 	return ret;
 }
@@ -403,25 +407,11 @@ EXPORT_SYMBOL_GPL(platform_device_add);
  */
 void platform_device_del(struct platform_device *pdev)
 {
-	int i;
-
-	if (pdev) {
-		device_del(&pdev->dev);
-
-		if (pdev->id_auto) {
-			ida_simple_remove(&platform_devid_ida, pdev->id);
-			pdev->id = PLATFORM_DEVID_AUTO;
-		}
-
-		for (i = 0; i < pdev->num_resources; i++) {
-			struct resource *r = &pdev->resource[i];
-			unsigned long type = resource_type(r);
+	if (!pdev)
+		return;
 
-			if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
-					r->parent)
-				release_resource(r);
-		}
-	}
+	device_del(&pdev->dev);
+	platform_device_cleanout(pdev, pdev->num_resources);
 }
 EXPORT_SYMBOL_GPL(platform_device_del);
 
-- 
2.1.4