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

[PATCH 1/2 v2] 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

[PATCH 2/2 v2] 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 2/2 v2] of/platform: Use platform_device interface

[Rob Herring]

On Tue, Apr 21, 2015 at 3:25 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 is because some DTs have overlapping resources and doing this
would break things. If you look at the git history, this was fixed and
then reverted by Grant.

Rob

> 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 2/2 v2] of/platform: Use platform_device interface

[Ricardo Ribalda Delgado]

Hello Rob

On Tue, Apr 21, 2015 at 10:13 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, Apr 21, 2015 at 3:25 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 is because some DTs have overlapping resources and doing this
> would break things. If you look at the git history, this was fixed and
> then reverted by Grant.

I cannot find that commit sorry, could you give me the hash or a link
to the mailing list?

ricardo@pilix:~/linux$ git shortlog drivers/of/platform.c | grep -i Revert
      Revert "drivers: of: add initialization code for dma reserved memory"


To give a litte context to this patch, the issue started with this
conversaion with Bjorn:
https://lkml.org/lkml/2015/4/20/435


What we have today is also wrong, it leads to a null pointer deference
(and therefore a whole crash).

If we cannot use platform_device_add, then we cannot use
platform_device_destroy :)

Shall I prepare a patch replacing platform_device_destroy()?


Thanks!

Re: [PATCH 2/2 v2] of/platform: Use platform_device interface

[Rob Herring]

On Tue, Apr 21, 2015 at 4:09 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Hello Rob
>
> On Tue, Apr 21, 2015 at 10:13 PM, Rob Herring <robherring2@gmail.com> wrote:
>> On Tue, Apr 21, 2015 at 3:25 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 is because some DTs have overlapping resources and doing this
>> would break things. If you look at the git history, this was fixed and
>> then reverted by Grant.
>
> I cannot find that commit sorry, could you give me the hash or a link
> to the mailing list?
>
> ricardo@pilix:~/linux$ git shortlog drivers/of/platform.c | grep -i Revert
>       Revert "drivers: of: add initialization code for dma reserved memory"

commit 02bbde7849e68e193cefaa1885fe0df0f03c9fcd
Author: Grant Likely <grant.likely@secretlab.ca>
Date:   Sun Feb 17 20:03:27 2013 +0000

    Revert "of: use platform_device_add"

    This reverts commit aac73f34542bc7ae4317928d2eabfeb21d247323. That
    commit causes two kinds of breakage; it breaks registration of AMBA
    devices when one of the parent nodes already contains overlapping
    resource regions, and it breaks calls to request_region() by device
    drivers in certain conditions where there are overlapping memory
    regions. Both of these problems can probably be fixed, but it is better
    to back out the commit and get a proper fix designed before trying again.

    Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

>
>
> To give a litte context to this patch, the issue started with this
> conversaion with Bjorn:
> https://lkml.org/lkml/2015/4/20/435
>
>
> What we have today is also wrong, it leads to a null pointer deference
> (and therefore a whole crash).
>
> If we cannot use platform_device_add, then we cannot use
> platform_device_destroy :)
>
> Shall I prepare a patch replacing platform_device_destroy()?

Perhaps we make inserting resource failure non-fatal so by default we
can have resources inserted but not break the cases Grant mentioned.
Ideally we want to not have new platforms with overlapping resources
in the DT.

Rob

Re: [PATCH 2/2 v2] of/platform: Use platform_device interface

[Grant Likely]

On Tue, 21 Apr 2015 18:01:00 -0500
, Rob Herring <robherring2@gmail.com>
 wrote:
> On Tue, Apr 21, 2015 at 4:09 PM, Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
> > Hello Rob
> >
> > On Tue, Apr 21, 2015 at 10:13 PM, Rob Herring <robherring2@gmail.com> wrote:
> >> On Tue, Apr 21, 2015 at 3:25 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 is because some DTs have overlapping resources and doing this
> >> would break things. If you look at the git history, this was fixed and
> >> then reverted by Grant.
> >
> > I cannot find that commit sorry, could you give me the hash or a link
> > to the mailing list?
> >
> > ricardo@pilix:~/linux$ git shortlog drivers/of/platform.c | grep -i Revert
> >       Revert "drivers: of: add initialization code for dma reserved memory"
> 
> commit 02bbde7849e68e193cefaa1885fe0df0f03c9fcd
> Author: Grant Likely <grant.likely@secretlab.ca>
> Date:   Sun Feb 17 20:03:27 2013 +0000
> 
>     Revert "of: use platform_device_add"
> 
>     This reverts commit aac73f34542bc7ae4317928d2eabfeb21d247323. That
>     commit causes two kinds of breakage; it breaks registration of AMBA
>     devices when one of the parent nodes already contains overlapping
>     resource regions, and it breaks calls to request_region() by device
>     drivers in certain conditions where there are overlapping memory
>     regions. Both of these problems can probably be fixed, but it is better
>     to back out the commit and get a proper fix designed before trying again.
> 
>     Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> 
> >
> >
> > To give a litte context to this patch, the issue started with this
> > conversaion with Bjorn:
> > https://lkml.org/lkml/2015/4/20/435
> >
> >
> > What we have today is also wrong, it leads to a null pointer deference
> > (and therefore a whole crash).
> >
> > If we cannot use platform_device_add, then we cannot use
> > platform_device_destroy :)
> >
> > Shall I prepare a patch replacing platform_device_destroy()?
> 
> Perhaps we make inserting resource failure non-fatal so by default we
> can have resources inserted but not break the cases Grant mentioned.
> Ideally we want to not have new platforms with overlapping resources
> in the DT.

I think I tried that too and then ran in to a problem where drivers will
fail if a different device 'owns' the resource.

For example, if device-a and device-b have overlapping regions, device-a
gets registered first and claims the region. device-b tries to claim the
region, fails, but is allowed to continue anyway. When driver-b tries to
bind to device-b, and does a request_resource(), it will fail because
device-a already owns it. I don't have a good solution for that other
than to completely disable insert_resource() when using devicetree.

g.

Re: [PATCH 2/2 v2] of/platform: Use platform_device interface

[Ricardo Ribalda Delgado]

Hello Grant

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

>
> I think I tried that too and then ran in to a problem where drivers will
> fail if a different device 'owns' the resource.
>
> For example, if device-a and device-b have overlapping regions, device-a
> gets registered first and claims the region. device-b tries to claim the
> region, fails, but is allowed to continue anyway. When driver-b tries to
> bind to device-b, and does a request_resource(), it will fail because
> device-a already owns it. I don't have a good solution for that other
> than to completely disable insert_resource() when using devicetree.

If someone is following this conversation I have already replied in

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

Message-ID: <CAPybu_3gYAZTHHYD7y2MdKFJBwDVyb5a9fwQqEMc+0xmKSTpKg@mail.gmail.com>

Regards!

>
> g.



-- 
Ricardo Ribalda

Re: [PATCH 2/2 v2] of/platform: Use platform_device interface

[Ricardo Ribalda Delgado]

Hello Grant and Rob

Could you take a look to these two patches that I have just sent.

kernel/resource: Add new flag IORESOURCE_SHARED
of/platform: Mark all device tree resources as SHARED

I think it fixes Grant problem.


Regards!