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

[PATCH] 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.

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..35dc716 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 (!old->parent)
+		return -EINVAL;
+
 	p = &old->parent->child;
 	for (;;) {
 		tmp = *p;
-- 
2.1.4

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

[Bjorn Helgaas]

[+cc Grant (author of ac80a51e2ce5)]

Hi Ricardo,

On Mon, Apr 20, 2015 at 06:22:52PM +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.

Interesting; how'd you find this?  It looks like the
of_platform_depopulate() code has been this way for a long time, so we
must be doing something new that makes us trip over this now.  More
analysis below...

> This patch evaluates resouce->parent before accessing it. If it
> is not initialized, -EACCESS is returned.
> 
> 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..35dc716 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 (!old->parent)
> +		return -EINVAL;

This path has been fine for a long time without testing for a NULL
pointer, so I suspect this change papers over an issue that would be
better fixed elsewhere.

>From reading drivers/base/platform.c, it looks like the intent is
that platform device users would use these interfaces:

  - platform_device_alloc()

  - platform_device_add_resources(platform_device *pdev)
      pdev->num_resources = num

  - platform_device_add(platform_device *pdev)
      for (i = 0; i < pdev->num_resources; i++)
	insert_resource()
      device_add(&pdev->dev)

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

Resources are added by platform_device_add_resources() and inserted
into the resource tree by platform_device_add().  In this usage,
resources are removed from the resource tree by
platform_device_unregister(), and there's no issue with
resource->parent being NULL.

OF uses platform_device_alloc() and platform_device_unregister(), but 
not platform_device_add().  It doesn't call insert_resource(), and that
breaks the platform_device_unregister() assumption that the resources
are in the resource tree:

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

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

I cc'd Grant because ac80a51e2ce5 ("of/device: populate
platform_device (of_device) resource table on allocation") added the
pdev->resource management in of_device_alloc(), so maybe he has more
insight into this.

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

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

[Ricardo Ribalda Delgado]

Hi Bjorn!

Thanks for your promtly response.

On Mon, Apr 20, 2015 at 9:28 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Grant (author of ac80a51e2ce5)]
>
> Hi Ricardo,
>
> On Mon, Apr 20, 2015 at 06:22:52PM +0200, Ricardo Ribalda Delgado wrote:
>>
>> If of_platform_depopulate is called later, resource->parent is
>> accessed (Offset 0x30 of address 0), causing a kernel error.
>
> Interesting; how'd you find this?  It looks like the
> of_platform_depopulate() code has been this way for a long time, so we
> must be doing something new that makes us trip over this now.  More
> analysis below...

I have an out of tree driver that dynamically adds devices to the device tree.

It was developed before the dynamic_of and dt_overlays existed. Now I
am porting my code to the new interfaces available. I am trying to do
it small steps.

First step was being able to depopulate a previously loaded device
tree. Old, code was calling of_platform_populate, so calling
of_platform_depopulate looked like the right choice. Unfortunately
everything crashed, and it turned out that this was the issue.

On my defense I would say, that the plan is to make this driver
public, once the hardware is stabilized and sold to the public.

>> @@ -237,6 +237,9 @@ static int __release_resource(struct resource *old)
>>  {
>>       struct resource *tmp, **p;
>>
>> +     if (!old->parent)
>> +             return -EINVAL;
>
> This path has been fine for a long time without testing for a NULL
> pointer, so I suspect this change papers over an issue that would be
> better fixed elsewhere.
>

This code is pretty tested, but dynamic remove is not.

> From reading drivers/base/platform.c, it looks like the intent is
> that platform device users would use these interfaces:


I can take a look to modify OF to use insert_resource(), but I still
think that no matter what, we should add this extra check, like the
propossed patch or maybe with a BUG_ON()....


Lets see what Grant thinks about this.


Thanks again!



-- 
Ricardo Ribalda

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

[Bjorn Helgaas]

On Mon, Apr 20, 2015 at 10:24:25PM +0200, Ricardo Ribalda Delgado wrote:
> Hi Bjorn!
> 
> Thanks for your promtly response.
> 
> On Mon, Apr 20, 2015 at 9:28 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > [+cc Grant (author of ac80a51e2ce5)]
> >
> > Hi Ricardo,
> >
> > On Mon, Apr 20, 2015 at 06:22:52PM +0200, Ricardo Ribalda Delgado wrote:
> >>
> >> If of_platform_depopulate is called later, resource->parent is
> >> accessed (Offset 0x30 of address 0), causing a kernel error.
> >
> > Interesting; how'd you find this?  It looks like the
> > of_platform_depopulate() code has been this way for a long time, so we
> > must be doing something new that makes us trip over this now.  More
> > analysis below...
> 
> I have an out of tree driver that dynamically adds devices to the device tree.
> 
> It was developed before the dynamic_of and dt_overlays existed. Now I
> am porting my code to the new interfaces available. I am trying to do
> it small steps.
> 
> First step was being able to depopulate a previously loaded device
> tree. Old, code was calling of_platform_populate, so calling
> of_platform_depopulate looked like the right choice. Unfortunately
> everything crashed, and it turned out that this was the issue.
> 
> On my defense I would say, that the plan is to make this driver
> public, once the hardware is stabilized and sold to the public.

No need to defend yourself; to me this looks like a bug in the
of_platform code, so it's a good thing you tripped over it :)

The obvious bug is the NULL pointer dereference.  The not-quite-so-
obvious bug is that I think the lack of insert_resource() means the
resource tree (/proc/iomem, /proc/ioports) is missing some useful
information.

> > From reading drivers/base/platform.c, it looks like the intent is
> > that platform device users would use these interfaces:
> 
> I can take a look to modify OF to use insert_resource(), but I still
> think that no matter what, we should add this extra check, like the
> propossed patch or maybe with a BUG_ON()....

I think it would be nicer to make OF use platform_device_add_resources()
and platform_device_add() because then there's less duplication of code.
But Grant might have had a reason for avoiding that.

Bjorn

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

[Ricardo Ribalda Delgado]

Hello

On Mon, Apr 20, 2015 at 10:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

>> > From reading drivers/base/platform.c, it looks like the intent is
>> > that platform device users would use these interfaces:
>>
>> I can take a look to modify OF to use insert_resource(), but I still
>> think that no matter what, we should add this extra check, like the
>> propossed patch or maybe with a BUG_ON()....
>
> I think it would be nicer to make OF use platform_device_add_resources()
> and platform_device_add() because then there's less duplication of code.
> But Grant might have had a reason for avoiding that.
>
> Bjorn

I think I am going to make two patches, one  modifying OF as you
suggest, and another one
adding a BUG_ON to release_resource. Then you can decide to apply one
or two with the feedback from Grant

I have no hardware to test it right now, but tomorrow morning I can
test the patches and send them.


Thanks!
-- 
Ricardo Ribalda

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

[Thierry Reding]

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

On Mon, Apr 20, 2015 at 10:49:28PM +0200, Ricardo Ribalda Delgado wrote:
> Hello
> 
> On Mon, Apr 20, 2015 at 10:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 
> >> > From reading drivers/base/platform.c, it looks like the intent is
> >> > that platform device users would use these interfaces:
> >>
> >> I can take a look to modify OF to use insert_resource(), but I still
> >> think that no matter what, we should add this extra check, like the
> >> propossed patch or maybe with a BUG_ON()....
> >
> > I think it would be nicer to make OF use platform_device_add_resources()
> > and platform_device_add() because then there's less duplication of code.
> > But Grant might have had a reason for avoiding that.
> >
> > Bjorn
> 
> I think I am going to make two patches, one  modifying OF as you
> suggest, and another one
> adding a BUG_ON to release_resource. Then you can decide to apply one
> or two with the feedback from Grant

I don't see the point in using BUG_ON() here. That's going to crash the
system anyway, so you could just as well let it crash while it's trying
to dereference the parent pointer. Maybe a WARN_ON() along with an
appropriate error code would be better here.

As to the underlying problem, perhaps of_device_add() should be calling
platform_device_add() rather than device_add()? Essentially the OF glue
creates regular platform devices, so I think it should be reusing as
much of the platform code as possible. From a quick look that might be
somewhat hairy to do, but on the other hand these kinds of problems are
bound to happen over and over again because both implementations of
platform devices need to be manually kept in sync.

Thierry

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