[PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Schmid, Carsten]

When a resource is freed and has children, the childrens are
left without any hint that their parent is no more valid.
This caused at least one use-after-free in the xhci-hcd using
ext-caps driver when platform code released platform devices.

Fix this by setting child's parent to zero and warn.

Signed-off-by: Carsten Schmid <carsten_schmid@mentor.com>
---
Rationale:
When hunting for the root cause of a crash on a 4.14.86 kernel, i
have found the root cause and checked it being still present
upstream. Our case:
Having xhci-hcd and intel_xhci_usb_sw active we can see in
/proc/meminfo: (exceirpt)
  b3c00000-b3c0ffff : 0000:00:15.0
    b3c00000-b3c0ffff : xhci-hcd
      b3c08070-b3c0846f : intel_xhci_usb_sw
intel_xhci_usb_sw being a child of xhci-hcd.

Doing an unbind command
echo 0000:00:15.0 > /sys/bus/pci/drivers/xhci_hcd/unbind
leads to xhci-hcd being freed in __release_region.
The intel_xhci_usb_sw resource is accessed in platform code
in platform_device_del with
		for (i = 0; i < pdev->num_resources; i++) {
			struct resource *r = &pdev->resource[i];
			if (r->parent)
				release_resource(r);
		}
as the resource's parent has not been updated, the release_resource
uses the parent:
	p = &old->parent->child;
which is now invalid.
Fix this by marking the parent invalid in the child and give a warning
in dmesg.
---
 kernel/resource.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index 158f04ec1d4f..95340cb0b1c2 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1200,6 +1200,15 @@ void __release_region(struct resource *parent, resource_size_t start,
 			write_unlock(&resource_lock);
 			if (res->flags & IORESOURCE_MUXED)
 				wake_up(&muxed_resource_wait);
+
+			write_lock(&resource_lock);
+			if (res->child) {
+				printk(KERN_WARNING "__release_region: %s has child %s,"
+						"invalidating childs parent\n",
+						res->name, res->child->name);
+				res->child->parent = NULL;
+			}
+			write_unlock(&resource_lock);
 			free_resource(res);
 			return;
 		}
-- 
2.17.1

Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Schmid, Carsten]

When a resource is freed and has children, the childrens are
left without any hint that their parent is no more valid.
This caused at least one use-after-free in the xhci-hcd using
ext-caps driver when platform code released platform devices.

Fix this by setting child's parent to zero and warn.

Signed-off-by: Carsten Schmid <carsten_schmid@mentor.com>
---
Rationale:
When hunting for the root cause of a crash on a 4.14.86 kernel, i
have found the root cause and checked it being still present
upstream. Our case:
Having xhci-hcd and intel_xhci_usb_sw active we can see in
/proc/meminfo: (exceirpt)
  b3c00000-b3c0ffff : 0000:00:15.0
    b3c00000-b3c0ffff : xhci-hcd
      b3c08070-b3c0846f : intel_xhci_usb_sw
intel_xhci_usb_sw being a child of xhci-hcd.

Doing an unbind command
echo 0000:00:15.0 > /sys/bus/pci/drivers/xhci_hcd/unbind
leads to xhci-hcd being freed in __release_region.
The intel_xhci_usb_sw resource is accessed in platform code
in platform_device_del with
                for (i = 0; i < pdev->num_resources; i++) {
                        struct resource *r = &pdev->resource[i];
                        if (r->parent)
                                release_resource(r);
                }
as the resource's parent has not been updated, the release_resource
uses the parent:
        p = &old->parent->child;
which is now invalid.
Fix this by marking the parent invalid in the child and give a warning
in dmesg.
---
Advised by Greg (thanks):
Try resending it with at least the people who get_maintainer.pl says has
touched that file last in it. [CS:done]

Also, Linus is the unofficial resource.c maintainer.  I think he has a
set of userspace testing scripts for changes somewhere, so you should
 cc: him too.  And might as well add me :) [CS:done]

 thanks,

 greg k-h
---
 kernel/resource.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index 158f04ec1d4f..95340cb0b1c2 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1200,6 +1200,15 @@ void __release_region(struct resource *parent, resource_size_t start,
                        write_unlock(&resource_lock);
                        if (res->flags & IORESOURCE_MUXED)
                                wake_up(&muxed_resource_wait);
+
+                       write_lock(&resource_lock);
+                       if (res->child) {
+                               printk(KERN_WARNING "__release_region: %s has child %s,"
+                                               "invalidating childs parent\n",
+                                               res->name, res->child->name);
+                               res->child->parent = NULL;
+                       }
+                       write_unlock(&resource_lock);
                        free_resource(res);
                        return;
                }
--
2.17.1

Re: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Dan Williams]

On Fri, Aug 9, 2019 at 6:50 AM Schmid, Carsten
<Carsten_Schmid@mentor.com> wrote:
>
> When a resource is freed and has children, the childrens are
> left without any hint that their parent is no more valid.
> This caused at least one use-after-free in the xhci-hcd using
> ext-caps driver when platform code released platform devices.
>
> Fix this by setting child's parent to zero and warn.
>
> Signed-off-by: Carsten Schmid <carsten_schmid@mentor.com>
> ---
> Rationale:
> When hunting for the root cause of a crash on a 4.14.86 kernel, i
> have found the root cause and checked it being still present
> upstream. Our case:
> Having xhci-hcd and intel_xhci_usb_sw active we can see in
> /proc/meminfo: (exceirpt)
>   b3c00000-b3c0ffff : 0000:00:15.0
>     b3c00000-b3c0ffff : xhci-hcd
>       b3c08070-b3c0846f : intel_xhci_usb_sw
> intel_xhci_usb_sw being a child of xhci-hcd.
>
> Doing an unbind command
> echo 0000:00:15.0 > /sys/bus/pci/drivers/xhci_hcd/unbind
> leads to xhci-hcd being freed in __release_region.
> The intel_xhci_usb_sw resource is accessed in platform code
> in platform_device_del with
>                 for (i = 0; i < pdev->num_resources; i++) {
>                         struct resource *r = &pdev->resource[i];
>                         if (r->parent)
>                                 release_resource(r);
>                 }

How did we get here while intel_xhci_usb_sw is still active? I would
have expected intel_xhci_usb_sw to pin its parent preventing release
while any usage was pending?

> as the resource's parent has not been updated, the release_resource
> uses the parent:
>         p = &old->parent->child;
> which is now invalid.
> Fix this by marking the parent invalid in the child and give a warning

This does not seem like a fix. It does seem like a good sanity check
though, some notes below.

> in dmesg.
> ---
> Advised by Greg (thanks):
> Try resending it with at least the people who get_maintainer.pl says has
> touched that file last in it. [CS:done]
>
> Also, Linus is the unofficial resource.c maintainer.  I think he has a
> set of userspace testing scripts for changes somewhere, so you should
>  cc: him too.  And might as well add me :) [CS:done]
>
>  thanks,
>
>  greg k-h
> ---
>  kernel/resource.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 158f04ec1d4f..95340cb0b1c2 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1200,6 +1200,15 @@ void __release_region(struct resource *parent, resource_size_t start,
>                         write_unlock(&resource_lock);
>                         if (res->flags & IORESOURCE_MUXED)
>                                 wake_up(&muxed_resource_wait);
> +
> +                       write_lock(&resource_lock);

I'd move this above that write_unlock() a few lines up to eliminate a
lock bounce.

> +                       if (res->child) {
> +                               printk(KERN_WARNING "__release_region: %s has child %s,"

How about WARN_ONCE() to identify the code path that mis-sequenced the release?

> +                                               "invalidating childs parent\n",

s/childs/child's/

> +                                               res->name, res->child->name);
> +                               res->child->parent = NULL;
> +                       }
> +                       write_unlock(&resource_lock);
>                         free_resource(res);
>                         return;
>                 }
> --
> 2.17.1

Re: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Joe Perches]

On Fri, 2019-08-09 at 13:50 +0000, Schmid, Carsten wrote:
> When a resource is freed and has children, the childrens are
> left without any hint that their parent is no more valid.
> This caused at least one use-after-free in the xhci-hcd using
> ext-caps driver when platform code released platform devices.
> 
> Fix this by setting child's parent to zero and warn.
> 
> Signed-off-by: Carsten Schmid <carsten_schmid@mentor.com>
> ---
> Rationale:
> When hunting for the root cause of a crash on a 4.14.86 kernel, i
> have found the root cause and checked it being still present
> upstream. Our case:
> Having xhci-hcd and intel_xhci_usb_sw active we can see in
> /proc/meminfo: (exceirpt)
>   b3c00000-b3c0ffff : 0000:00:15.0
>     b3c00000-b3c0ffff : xhci-hcd
>       b3c08070-b3c0846f : intel_xhci_usb_sw
> intel_xhci_usb_sw being a child of xhci-hcd.
> 
> Doing an unbind command
> echo 0000:00:15.0 > /sys/bus/pci/drivers/xhci_hcd/unbind
> leads to xhci-hcd being freed in __release_region.
> The intel_xhci_usb_sw resource is accessed in platform code
> in platform_device_del with
>                 for (i = 0; i < pdev->num_resources; i++) {
>                         struct resource *r = &pdev->resource[i];
>                         if (r->parent)
>                                 release_resource(r);
>                 }
> as the resource's parent has not been updated, the release_resource
> uses the parent:
>         p = &old->parent->child;
> which is now invalid.
> Fix this by marking the parent invalid in the child and give a warning
> in dmesg.
> ---
> Advised by Greg (thanks):
> Try resending it with at least the people who get_maintainer.pl says has
> touched that file last in it. [CS:done]
> 
> Also, Linus is the unofficial resource.c maintainer.  I think he has a
> set of userspace testing scripts for changes somewhere, so you should
>  cc: him too.  And might as well add me :) [CS:done]
[]
> diff --git a/kernel/resource.c b/kernel/resource.c
[]
> @@ -1200,6 +1200,15 @@ void __release_region(struct resource *parent, resource_size_t start,
>                         write_unlock(&resource_lock);
>                         if (res->flags & IORESOURCE_MUXED)
>                                 wake_up(&muxed_resource_wait);
> +
> +                       write_lock(&resource_lock);
> +                       if (res->child) {
> +                               printk(KERN_WARNING "__release_region: %s has child %s,"
> +                                               "invalidating childs parent\n",
> +                                               res->name, res->child->name);

Please coalesce the format because there is likely an unintentional
missing space after the comma, and use pr_warn, %s and __func__

				pr_warn("%s: %s has child %s, invalidating child's parent\n",
					__func__, res->name, res->child->name);

[PATCH] kernel/resource.c: Convert printks to pr_<level>

[Joe Perches]

Use pr_<level> to get the output prefixed by the existing #define pr_fmt

Miscellanea:

o Convert bare printk to pr_info
o Reduce printk(KERN_WARNING to pr_info as the log level seems better

Signed-off-by: Joe Perches <joe@perches.com>
---
 kernel/resource.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 7ea4306503c5..59b89e40502a 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -901,7 +901,8 @@ void insert_resource_expand_to_fit(struct resource *root, struct resource *new)
 		if (conflict->end > new->end)
 			new->end = conflict->end;
 
-		printk("Expanded resource %s due to conflict with %s\n", new->name, conflict->name);
+		pr_info("Expanded resource %s due to conflict with %s\n",
+			new->name, conflict->name);
 	}
 	write_unlock(&resource_lock);
 }
@@ -1223,9 +1224,8 @@ void __release_region(struct resource *parent, resource_size_t start,
 
 	write_unlock(&resource_lock);
 
-	printk(KERN_WARNING "Trying to free nonexistent resource "
-		"<%016llx-%016llx>\n", (unsigned long long)start,
-		(unsigned long long)end);
+	pr_warn("Trying to free nonexistent resource <%016llx-%016llx>\n",
+		(unsigned long long)start, (unsigned long long)end);
 }
 EXPORT_SYMBOL(__release_region);
 
@@ -1557,10 +1557,10 @@ int iomem_map_sanity_check(resource_size_t addr, unsigned long size)
 		if (p->flags & IORESOURCE_BUSY)
 			continue;
 
-		printk(KERN_WARNING "resource sanity check: requesting [mem %#010llx-%#010llx], which spans more than %s %pR\n",
-		       (unsigned long long)addr,
-		       (unsigned long long)(addr + size - 1),
-		       p->name, p);
+		pr_info("sanity check: requesting [mem %#010llx-%#010llx], which spans more than %s %pR\n",
+			(unsigned long long)addr,
+			(unsigned long long)(addr + size - 1),
+			p->name, p);
 		err = -1;
 		break;
 	}

Re: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Linus Torvalds]

On Fri, Aug 9, 2019 at 6:50 AM Schmid, Carsten
<Carsten_Schmid@mentor.com> wrote:
>
> @@ -1200,6 +1200,15 @@ void __release_region(struct resource *parent, resource_size_t start,
>                         write_unlock(&resource_lock);
>                         if (res->flags & IORESOURCE_MUXED)
>                                 wake_up(&muxed_resource_wait);
> +
> +                       write_lock(&resource_lock);
> +                       if (res->child) {
> +                               printk(KERN_WARNING "__release_region: %s has child %s,"
> +                                               "invalidating childs parent\n",
> +                                               res->name, res->child->name);
> +                               res->child->parent = NULL;
> +                       }
> +                       write_unlock(&resource_lock);
>                         free_resource(res);

So I think that this should be inside the previous resource_lock, and
before the whole "wake up muxed resource".

Also, a few other issues:

 - what about other freeing cases?  I'm looking at

        release_mem_region_adjustable()

   which has the same pattern where a resource may be freed.

 - what about multiple children? Your patch sets res->child->parent to
NULL, but what about possible other children (iow, the
res->child->sibling list)

 - releasing a resource without having released its children is a
nasty bug, but the bug is now here in release_region, it is in the
*caller*. The printk() (or pr_warn()) doesn't really help find that.

So my gut feel is that this patch is a symptom of a real bug, and a
warning is worthwhile to fix that bug, but more thought is needed.

Maybe something more along the line of

    diff --git a/kernel/resource.c b/kernel/resource.c
    index 7ea4306503c5..ebe06d77b06a 100644
    --- a/kernel/resource.c
    +++ b/kernel/resource.c
    @@ -1211,6 +1211,8 @@ void __release_region(struct resource
*parent, resource_size_t start,
                        }
                        if (res->start != start || res->end != end)
                                break;
    +                   if (WARN_ON_ONCE(res->child))
    +                           break;
                        *p = res->sibling;
                        write_unlock(&resource_lock);
                        if (res->flags & IORESOURCE_MUXED)

would be more appropriate? It simply refuses to free a resource that
has children, and gives a warning (with a backtrace) for the situation
(since clearly we now end up with a resource leak).

                Linus

Re: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Wei Yang]

On Fri, Aug 09, 2019 at 01:50:24PM +0000, Schmid, Carsten wrote:
>When a resource is freed and has children, the childrens are
>left without any hint that their parent is no more valid.
>This caused at least one use-after-free in the xhci-hcd using
>ext-caps driver when platform code released platform devices.
>
>Fix this by setting child's parent to zero and warn.
>
>Signed-off-by: Carsten Schmid <carsten_schmid@mentor.com>
>---
>Rationale:
>When hunting for the root cause of a crash on a 4.14.86 kernel, i
>have found the root cause and checked it being still present
>upstream. Our case:
>Having xhci-hcd and intel_xhci_usb_sw active we can see in
>/proc/meminfo: (exceirpt)
>  b3c00000-b3c0ffff : 0000:00:15.0
>    b3c00000-b3c0ffff : xhci-hcd
>      b3c08070-b3c0846f : intel_xhci_usb_sw
>intel_xhci_usb_sw being a child of xhci-hcd.
>
>Doing an unbind command
>echo 0000:00:15.0 > /sys/bus/pci/drivers/xhci_hcd/unbind
>leads to xhci-hcd being freed in __release_region.
>The intel_xhci_usb_sw resource is accessed in platform code
>in platform_device_del with
>                for (i = 0; i < pdev->num_resources; i++) {
>                        struct resource *r = &pdev->resource[i];
>                        if (r->parent)
>                                release_resource(r);
>                }
>as the resource's parent has not been updated, the release_resource
>uses the parent:
>        p = &old->parent->child;
>which is now invalid.
>Fix this by marking the parent invalid in the child and give a warning
>in dmesg.
>---
>Advised by Greg (thanks):
>Try resending it with at least the people who get_maintainer.pl says has
>touched that file last in it. [CS:done]
>
>Also, Linus is the unofficial resource.c maintainer.  I think he has a
>set of userspace testing scripts for changes somewhere, so you should
> cc: him too.  And might as well add me :) [CS:done]
>
> thanks,
>
> greg k-h
>---
> kernel/resource.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/kernel/resource.c b/kernel/resource.c
>index 158f04ec1d4f..95340cb0b1c2 100644
>--- a/kernel/resource.c
>+++ b/kernel/resource.c
>@@ -1200,6 +1200,15 @@ void __release_region(struct resource *parent, resource_size_t start,
>                        write_unlock(&resource_lock);
>                        if (res->flags & IORESOURCE_MUXED)
>                                wake_up(&muxed_resource_wait);
>+
>+                       write_lock(&resource_lock);
>+                       if (res->child) {

In theory, child may have siblings. Would it be possible to have several
devices under xhci-hcd?

>+                               printk(KERN_WARNING "__release_region: %s has child %s,"
>+                                               "invalidating childs parent\n",
>+                                               res->name, res->child->name);
>+                               res->child->parent = NULL;
>+                       }
>+                       write_unlock(&resource_lock);
>                        free_resource(res);
>                        return;
>                }
>--
>2.17.1

-- 
Wei Yang
Help you, Help me

Re: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Linus Torvalds]

On Fri, Aug 9, 2019 at 3:38 PM Wei Yang <richard.weiyang@gmail.com> wrote:
>
> In theory, child may have siblings. Would it be possible to have several
> devices under xhci-hcd?

I'm less interested in the xhci-hcd case - which I certainly *hope* is
fixed already? - than in "if this happens somewhere else".

So if we do want to remove the parent (which may be a good idea with a
warning), and want to make sure that the children are really removed
from the resource hierarchy, we should do somethiing like

  static bool detach_children(struct resource *res)
  {
        res = res->child;
        if (!res)
                return false;
        do {
                res->parent = NULL;
                res = res->sibling;
        } while (res);
        return true;
  }

and then we could write the __release_region() warning as

        /* You should not release a resource that has children */
        WARN_ON_ONCE(detach_children(res));

or something?

NOTE! The above is entirely untested, and written purely in my mail
reader. It might be seriously buggy, including not compiling, or doing
odd things. See it more as a "maybe something like this" code snippet
example than any kind of final form.

               Linus

Re: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Wei Yang]

On Fri, Aug 09, 2019 at 03:45:59PM -0700, Linus Torvalds wrote:
>On Fri, Aug 9, 2019 at 3:38 PM Wei Yang <richard.weiyang@gmail.com> wrote:
>>
>> In theory, child may have siblings. Would it be possible to have several
>> devices under xhci-hcd?
>
>I'm less interested in the xhci-hcd case - which I certainly *hope* is
>fixed already? - than in "if this happens somewhere else".
>

Agree, this is what I want to say.

>So if we do want to remove the parent (which may be a good idea with a
>warning), and want to make sure that the children are really removed
>from the resource hierarchy, we should do somethiing like
>
>  static bool detach_children(struct resource *res)
>  {
>        res = res->child;
>        if (!res)
>                return false;
>        do {
>                res->parent = NULL;
>                res = res->sibling;
>        } while (res);
>        return true;
>  }
>
>and then we could write the __release_region() warning as
>
>        /* You should not release a resource that has children */
>        WARN_ON_ONCE(detach_children(res));
>

I am thinking about why this could happen.

To guard the core kernel code, it looks reasonable.

>or something?
>
>NOTE! The above is entirely untested, and written purely in my mail
>reader. It might be seriously buggy, including not compiling, or doing
>odd things. See it more as a "maybe something like this" code snippet
>example than any kind of final form.
>
>               Linus

-- 
Wei Yang
Help you, Help me

AW: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Schmid, Carsten]

> -----Ursprüngliche Nachricht-----
> Von: Wei Yang [mailto:richard.weiyang@gmail.com]
> Gesendet: Samstag, 10. August 2019 02:45
> An: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Wei Yang <richard.weiyang@gmail.com>; Schmid, Carsten
> <Carsten_Schmid@mentor.com>; bp@suse.de; dan.j.williams@intel.com;
> mingo@kernel.org; dave.hansen@linux.intel.com; linux-
> kernel@vger.kernel.org; bhelgaas@google.com; osalvador@suse.de;
> rdunlap@infradead.org; richardw.yang@linux.intel.com;
> gregkh@linuxfoundation.org
> Betreff: Re: Resend [PATCH] kernel/resource.c: invalidate parent when
> freed resource has childs
> 
> On Fri, Aug 09, 2019 at 03:45:59PM -0700, Linus Torvalds wrote:
> >On Fri, Aug 9, 2019 at 3:38 PM Wei Yang <richard.weiyang@gmail.com>
> wrote:
> >>
> >> In theory, child may have siblings. Would it be possible to have several
> >> devices under xhci-hcd?
> >
> >I'm less interested in the xhci-hcd case - which I certainly *hope* is
> >fixed already? - than in "if this happens somewhere else".
> >
> 
> Agree, this is what I want to say.
> 
Unfortunately this xhci-hcd case is not fixed yet.
I'm working on a driver fix proposal, discussing with Hans de Goede who
wrote the intel_xhci_usb_sw platform driver.

For me there is nothing invalid in the intel_xhci_usb_sw driver.
It is initialized from xhci-hcd/xhci-pci in 
  xhci_pci_probe --> xhci_ext_cap_init --> xhci_create_intel_xhci_sw_pdev
which then does
  devm_add_action_or_reset(dev, xhci_intel_unregister_pdev, pdev)

So far, so good. Doesn't look bad.

When unbinding the xhci-hcd driver, i can see that xhci_pci_remove executes,
and after this the xhci_intel_unregister_pdev is called.
This is what fails because xhci_pci_remove removes the parent of the resource created
in the xhci_create_intel_xhci_sw_pdev.
So the "devm framework" which is used here fails in this specific case.
Very unintentional.
The proposal will call xhci_intel_unregister_pdev from within the xhci-pci in a similar way
like the driver is created. So we will have the child killed before the parent :)

> >So if we do want to remove the parent (which may be a good idea with a
> >warning), and want to make sure that the children are really removed
> >from the resource hierarchy, we should do somethiing like
> >
> >  static bool detach_children(struct resource *res)
> >  {
> >        res = res->child;
> >        if (!res)
> >                return false;
> >        do {
> >                res->parent = NULL;
> >                res = res->sibling;
> >        } while (res);
> >        return true;
> >  }
> >
> >and then we could write the __release_region() warning as
> >
> >        /* You should not release a resource that has children */
> >        WARN_ON_ONCE(detach_children(res));
> >
Fine for me, this extended sanity check. This didn't came up to my mind.
Because i have a reproducer, i can test it and send it as V2.
If you have any additional ideas, let me know.

> 
> I am thinking about why this could happen.
See above explanation.

> 
> To guard the core kernel code, it looks reasonable.
> 
Exactly my motivation :)

> >or something?
> >
> >NOTE! The above is entirely untested, and written purely in my mail
> >reader. It might be seriously buggy, including not compiling, or doing
> >odd things. See it more as a "maybe something like this" code snippet
> >example than any kind of final form.
> >
> >               Linus
> 
I'll implement and check it, of course. Development as usual.
Thanks!

Carsten
> --
> Wei Yang
> Help you, Help me

AW: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs

[Schmid, Carsten]

> >
> > In theory, child may have siblings. Would it be possible to have several
> > devices under xhci-hcd?
> 
> I'm less interested in the xhci-hcd case - which I certainly *hope* is
> fixed already? - than in "if this happens somewhere else".
> 
> So if we do want to remove the parent (which may be a good idea with a
> warning), and want to make sure that the children are really removed
> from the resource hierarchy, we should do somethiing like
> 
>   static bool detach_children(struct resource *res)
>   {
>         res = res->child;
>         if (!res)
>                 return false;
>         do {
>                 res->parent = NULL;
>                 res = res->sibling;
>         } while (res);
>         return true;
>   }
> 
> and then we could write the __release_region() warning as
> 
>         /* You should not release a resource that has children */
>         WARN_ON_ONCE(detach_children(res));
> 
> or something?
> 
... and a child may have children too ...

There is a __release_child_resources in resource.c around line 242.
A bit noisy, and does a similar thing you outlined above.
I'm thinking about to re-use that, but with more precise output
and WARN_ON_ONCE.

Suggestions before i start work on that?

Best regards
Carsten

[PATCH v2] kernel/resource.c: invalidate parent when freed resource has childs

[Schmid, Carsten]

When a resource is freed and has children, the childrens are
left without any hint that their parent is no more valid.
This caused at least one use-after-free in the xhci-hcd using
ext-caps driver when platform code released platform devices.

In such case, warn and release all resources beyond.

Signed-off-by: Carsten Schmid <carsten_schmid@mentor.com>
---
v2:
- release everything below the released resource, not only
  one child; re-using __release_child_resources
  (Inspired by Linus Torvalds outline)
- warn only once
  (According to Linus Torvalds outline)
- Keep parent and child name in warning message
  (eases hunting for the involved parties)
---
 kernel/resource.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index c3cc6d85ec52..eb48d793aa74 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -239,7 +239,7 @@ static int __release_resource(struct resource *old, bool release_child)
 	return -EINVAL;
 }
 
-static void __release_child_resources(struct resource *r)
+static void __release_child_resources(struct resource *r, bool warn)
 {
 	struct resource *tmp, *p;
 	resource_size_t size;
@@ -252,9 +252,10 @@ static void __release_child_resources(struct resource *r)
 
 		tmp->parent = NULL;
 		tmp->sibling = NULL;
-		__release_child_resources(tmp);
+		__release_child_resources(tmp, warn);
 
-		printk(KERN_DEBUG "release child resource %pR\n", tmp);
+		if (warn)
+			printk(KERN_DEBUG "release child resource %pR\n", tmp);
 		/* need to restore size, and keep flags */
 		size = resource_size(tmp);
 		tmp->start = 0;
@@ -265,7 +266,7 @@ static void __release_child_resources(struct resource *r)
 void release_child_resources(struct resource *r)
 {
 	write_lock(&resource_lock);
-	__release_child_resources(r);
+	__release_child_resources(r, true);
 	write_unlock(&resource_lock);
 }
 
@@ -1172,7 +1173,20 @@ EXPORT_SYMBOL(__request_region);
  * @n: resource region size
  *
  * The described resource region must match a currently busy region.
+ * If the region has children they are released too.
  */
+static void check_children(struct resource *parent)
+{
+	if (parent->child) {
+		/* warn and release all children */
+		WARN_ONCE(1, "%s: %s has child %s, release all children\n",
+				__func__, parent->name, parent->child->name);
+		write_lock(&resource_lock);
+		__release_child_resources(parent, false);
+		write_unlock(&resource_lock);
+	}
+}
+
 void __release_region(struct resource *parent, resource_size_t start,
 		      resource_size_t n)
 {
@@ -1200,6 +1214,10 @@ void __release_region(struct resource *parent, resource_size_t start,
 			write_unlock(&resource_lock);
 			if (res->flags & IORESOURCE_MUXED)
 				wake_up(&muxed_resource_wait);
+
+			/* You should'nt release a resource that has children */
+			check_children(res);
+
 			free_resource(res);
 			return;
 		}
-- 
2.17.1

Re: [PATCH v2] kernel/resource.c: invalidate parent when freed resource has childs

[Wei Yang]

On Wed, Aug 14, 2019 at 02:48:24PM +0000, Schmid, Carsten wrote:
>When a resource is freed and has children, the childrens are

s/childrens/children/

>left without any hint that their parent is no more valid.
>This caused at least one use-after-free in the xhci-hcd using
>ext-caps driver when platform code released platform devices.
>
>In such case, warn and release all resources beyond.
>
>Signed-off-by: Carsten Schmid <carsten_schmid@mentor.com>
>---
>v2:
>- release everything below the released resource, not only
>  one child; re-using __release_child_resources
>  (Inspired by Linus Torvalds outline)
>- warn only once
>  (According to Linus Torvalds outline)
>- Keep parent and child name in warning message
>  (eases hunting for the involved parties)
>---
> kernel/resource.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
>diff --git a/kernel/resource.c b/kernel/resource.c
>index c3cc6d85ec52..eb48d793aa74 100644
>--- a/kernel/resource.c
>+++ b/kernel/resource.c
>@@ -239,7 +239,7 @@ static int __release_resource(struct resource *old, bool release_child)
> 	return -EINVAL;
> }
> 
>-static void __release_child_resources(struct resource *r)
>+static void __release_child_resources(struct resource *r, bool warn)
> {
> 	struct resource *tmp, *p;
> 	resource_size_t size;
>@@ -252,9 +252,10 @@ static void __release_child_resources(struct resource *r)
> 
> 		tmp->parent = NULL;
> 		tmp->sibling = NULL;
>-		__release_child_resources(tmp);
>+		__release_child_resources(tmp, warn);

This function will release all the children.

Is this what Linus suggest?

From his code snippet, I just see siblings parent is set to NULL. I may miss
some point?

> 
>-		printk(KERN_DEBUG "release child resource %pR\n", tmp);
>+		if (warn)
>+			printk(KERN_DEBUG "release child resource %pR\n", tmp);
> 		/* need to restore size, and keep flags */
> 		size = resource_size(tmp);
> 		tmp->start = 0;
>@@ -265,7 +266,7 @@ static void __release_child_resources(struct resource *r)
> void release_child_resources(struct resource *r)
> {
> 	write_lock(&resource_lock);
>-	__release_child_resources(r);
>+	__release_child_resources(r, true);
> 	write_unlock(&resource_lock);
> }
> 
>@@ -1172,7 +1173,20 @@ EXPORT_SYMBOL(__request_region);
>  * @n: resource region size
>  *
>  * The described resource region must match a currently busy region.
>+ * If the region has children they are released too.
>  */
>+static void check_children(struct resource *parent)
>+{
>+	if (parent->child) {
>+		/* warn and release all children */
>+		WARN_ONCE(1, "%s: %s has child %s, release all children\n",
>+				__func__, parent->name, parent->child->name);
>+		write_lock(&resource_lock);

In previous version, lock is grasped before parent->child is checked.

Not sure why you change the order?

>+		__release_child_resources(parent, false);
>+		write_unlock(&resource_lock);
>+	}
>+}
>+
> void __release_region(struct resource *parent, resource_size_t start,
> 		      resource_size_t n)
> {
>@@ -1200,6 +1214,10 @@ void __release_region(struct resource *parent, resource_size_t start,
> 			write_unlock(&resource_lock);
> 			if (res->flags & IORESOURCE_MUXED)
> 				wake_up(&muxed_resource_wait);
>+
>+			/* You should'nt release a resource that has children */
>+			check_children(res);
>+
> 			free_resource(res);
> 			return;
> 		}
>-- 
>2.17.1
>

-- 
Wei Yang
Help you, Help me

AW: [PATCH v2] kernel/resource.c: invalidate parent when freed resource has childs

[Schmid, Carsten]

>>When a resource is freed and has children, the childrens are
> 
> s/childrens/children/
>
oh, missed that. Too many children ... ;-)
 
>>+		__release_child_resources(tmp, warn);
> 
> This function will release all the children.
> 
> Is this what Linus suggest?
> 
> From his code snippet, I just see siblings parent is set to NULL. I may miss
> some point?
>
At the point we are here, there should be no children, and children of
children at all ...
So they are all more or less lost in the wild.
That was why i didn't copy Linus' code 1:1 but reused an already existing
function doing similar thing.
It's anyway worth of thinking about this.

What i have in mind here (example):
Parent: iomem map 0x1000..0x1FFF
  Child1: iomem map 0x1000..0x17FF
    Child11: iomem map 0x1000..0x13FF
    Child12: iomem map 0x1400..0x17FF
  Child2: iomem map 0x1800..0x1FFF
    Child21: iomem map 0x1800..0x1BFF
    Child22: iomem map 0x1C00..0x1FFF

When releasing the parent, how can children 11, 12, 21 and 22 still be valid?
They don't know about their grandfather died ...
Looking at the __release_child_resources, i exactly found that all children are
invalidated/released in the way Linus did for the parent's children list.
Doesn't it make sense to do the same for all?

Please comment.

> >+static void check_children(struct resource *parent)
> >+{
> >+	if (parent->child) {
> >+		/* warn and release all children */
> >+		WARN_ONCE(1, "%s: %s has child %s, release all children\n",
> >+				__func__, parent->name, parent->child-
> >name);
> >+		write_lock(&resource_lock);
> 
> In previous version, lock is grasped before parent->child is checked.
> 
> Not sure why you change the order?
> 
To hold the lock as short as possible.
But yes, you are right, this could lead to problems if releasing of the
children is done in a parallel thread on a multicore ...
I'll change that to cover the whole resource access within the lock.
Not a big thing ...

Best regards
Carsten

Re: [PATCH v2] kernel/resource.c: invalidate parent when freed resource has childs

[Wei Yang]

On Thu, Aug 15, 2019 at 08:18:06AM +0000, Schmid, Carsten wrote:
>>>When a resource is freed and has children, the childrens are
>> 
>> s/childrens/children/
>>
>oh, missed that. Too many children ... ;-)
> 
>>>+		__release_child_resources(tmp, warn);
>> 
>> This function will release all the children.
>> 
>> Is this what Linus suggest?
>> 
>> From his code snippet, I just see siblings parent is set to NULL. I may miss
>> some point?
>>
>At the point we are here, there should be no children, and children of
>children at all ...
>So they are all more or less lost in the wild.
>That was why i didn't copy Linus' code 1:1 but reused an already existing
>function doing similar thing.
>It's anyway worth of thinking about this.
>
>What i have in mind here (example):
>Parent: iomem map 0x1000..0x1FFF
>  Child1: iomem map 0x1000..0x17FF
>    Child11: iomem map 0x1000..0x13FF
>    Child12: iomem map 0x1400..0x17FF
>  Child2: iomem map 0x1800..0x1FFF
>    Child21: iomem map 0x1800..0x1BFF
>    Child22: iomem map 0x1C00..0x1FFF
>
>When releasing the parent, how can children 11, 12, 21 and 22 still be valid?
>They don't know about their grandfather died ...
>Looking at the __release_child_resources, i exactly found that all children are
>invalidated/released in the way Linus did for the parent's children list.
>Doesn't it make sense to do the same for all?
>
>Please comment.
>
>> >+static void check_children(struct resource *parent)
>> >+{
>> >+	if (parent->child) {
>> >+		/* warn and release all children */
>> >+		WARN_ONCE(1, "%s: %s has child %s, release all children\n",
>> >+				__func__, parent->name, parent->child-
>> >name);
>> >+		write_lock(&resource_lock);
>> 
>> In previous version, lock is grasped before parent->child is checked.
>> 
>> Not sure why you change the order?
>> 
>To hold the lock as short as possible.
>But yes, you are right, this could lead to problems if releasing of the
>children is done in a parallel thread on a multicore ...
>I'll change that to cover the whole resource access within the lock.
>Not a big thing ...
>

My gut feeling is this is the problem from mal-functional driver, e.g.
xhci-hcd. We do our best to protect core kernel from it instead of do the
cleanup for it.

So my suggestion is to look into why xhci-hcd behave like this and fix that.

>Best regards
>Carsten

-- 
Wei Yang
Help you, Help me

AW: [PATCH v2] kernel/resource.c: invalidate parent when freed resource has childs

[Schmid, Carsten]

> My gut feeling is this is the problem from mal-functional driver, e.g.
> xhci-hcd. We do our best to protect core kernel from it instead of do the
> cleanup for it.
Agree.
My intention wasn't to fix mal-functional driver, but to give it a hint
that it's doing something wrong.
(In the xhci-hcd case the patch indirectly avoids the later use-after-free in driver,
 a nice side effect here)

I think the same what Linus meant with
> I'm less interested in the xhci-hcd case - which I certainly *hope* is
> fixed already? - than in "if this happens somewhere else".
What about giving only a WARN_ONCE?
Wouldn't hurt but notice developers and ease bug hunting.
Would be fine for me too.
Finally, i also could put the patch in a private branch named "useful_patches" ;-)
but then the community won't benefit.

> 
> So my suggestion is to look into why xhci-hcd behave like this and fix that.
> 
xhci-hcd fix proposal @ Hans de Goede already.

Best regards
Carsten

[PATCH] kernel/resource.c: warn if released region has children

[Schmid, Carsten]

When a resource region is released and has children,
the children are left without any hint that their
parent is no more valid.

This was observed on a use-after-free fault in the xhci-hcd
when xhci-hcd released his iomem region before
platform code released resources of platform devices
giving a random racy failure; one of the stacks observed:

[  230.412493] xhci_hcd 0000:00:15.0: USB bus 1 deregistered
[  230.416021] general protection fault: 0000 [#1] PREEMPT SMP NOPTI
[  230.422836] Modules linked in: bcmdhd(O-) ebtable_filter ebtables xfrm_user xfrm_algo cls_u32 sch_htb intel_tfm_governor cdc_acm ecryptfs intel_ipu4_psys intel_ipu4_psys_csslib snd_soc_apl_mgu_hu intel_xhci_usb_role_switch roles dwc3 adv728x udc_core snd_soc_skl sdw_cnl intel_ipu4_isys snd_soc_acpi_intel_match videobuf2_dma_contig videobuf2_memops snd_soc_acpi ipu4_acpi intel_ipu4_isys_csslib snd_soc_core snd_compress videobuf2_v4l2 videobuf2_core snd_soc_skl_ipc sdw_bus crc8 snd_soc_sst_ipc snd_soc_sst_dsp ahci snd_hda_ext_core coretemp snd_hda_core sbi_apl intel_ipu4_mmu i2c_i801 snd_pcm xhci_pci snd_timer xhci_hcd libahci cfg80211 mei_me snd usbcore libata intel_ipu4 rfkill soundcore usb_common mei dwc3_pci scsi_mod iova nfsd auth_rpcgss lockd grace sunrpc loop fuse 8021q bridge stp llc inap560t(O)
[  230.502258]  i915 video backlight intel_gtt i2c_algo_bit drm_kms_helper drm firmware_class igb_avb(O) ptp hwmon spi_pxa2xx_platform pps_core [last unloaded: bcmdhd]
[  230.518695] CPU: 1 PID: 20364 Comm: unbind-usb-str. Tainted: G     U     O    4.14.67-apl #1
[  230.528124] task: ffffa106ea869900 task.stack: ffffa24c84970000
[  230.534741] RIP: 0010:__release_resource+0x25/0x90
[  230.540090] RSP: 0018:ffffa24c84973c18 EFLAGS: 00010212
[  230.545924] RAX: f7410001b577e8a8 RBX: ffffa10731b4b700 RCX: ffffa10731b4a6c0
[  230.553893] RDX: f7410001b577e8a8 RSI: 0000000000000001 RDI: ffffa10731b4b700
[  230.561864] RBP: ffffa24c84973c18 R08: 0000000000000000 R09: 0000000000000000
[  230.569825] R10: ffffa24c8493b978 R11: 00000000000006c0 R12: ffffa106f1355000
[  230.577794] R13: ffffa24c84973c98 R14: ffffa24c84973c98 R15: 0000000000000001
[  230.585757] FS:  00007f365c48a740(0000) GS:ffffa10777c80000(0000) knlGS:0000000000000000
[  230.594794] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  230.601203] CR2: 000055562a2f0e88 CR3: 00000001f1108000 CR4: 00000000003406a0
[  230.609174] Call Trace:
[  230.611896]  release_resource+0x21/0x40
[  230.616181]  platform_device_del.part.1+0x4f/0x80
[  230.621433]  platform_device_unregister+0x12/0x20
[  230.626683]  xhci_intel_unregister_pdev+0x9/0x10 [xhci_hcd]
[  230.632906]  devm_action_release+0x10/0x20
[  230.637478]  release_nodes+0x10b/0x1f0
[  230.641663]  devres_release_all+0x37/0x50
[  230.646139]  device_release_driver_internal+0x19d/0x240
[  230.651974]  device_release_driver+0xd/0x10
[  230.656644]  unbind_store+0xb8/0x190
[  230.660633]  drv_attr_store+0x22/0x30
[  230.664720]  sysfs_kf_write+0x37/0x40
[  230.668807]  kernfs_fop_write+0x114/0x190
[  230.673283]  __vfs_write+0x35/0x160
[  230.677176]  vfs_write+0xb0/0x1a0
[  230.680873]  SyS_write+0x50/0xc0
[  230.684476]  do_syscall_64+0x79/0x340
[  230.688566]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[  230.694208] RIP: 0033:0x7f365bb91144
[  230.698198] RSP: 002b:00007ffe3da9eae8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[  230.706655] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f365bb91144
[  230.714628] RDX: 000000000000000d RSI: 000055562a2efe80 RDI: 0000000000000001
[  230.722599] RBP: 000055562a2efe80 R08: 000000000000000a R09: 00007f365be5acd0
[  230.730568] R10: 000000000000000a R11: 0000000000000246 R12: 00007f365be5b760
[  230.738538] R13: 000000000000000d R14: 00007f365be56760 R15: 000000000000000d
[  230.746501] Code: 84 00 00 00 00 00 48 8b 4f 28 55 b8 ea ff ff ff 48 89 e5 48 8b 51 38 48 85 d2 74 1d 48 39 d7 75 0a eb 62 48 39 c7 74 13 48 89 c2 <48> 8b 42 30 48 85 c0 75 ef b8 ea ff ff ff 5d c3 48 83 c2 30 40
[  230.767628] RIP: __release_resource+0x25/0x90 RSP: ffffa24c84973c18

Because the root cause - iomem area was released earlier -
could not be seen on analysis easily, a warning in the
release_region helps to detect such cases (if any exist).

xhci-hcd driver fix is issued separately.

Signed-off-by: Carsten Schmid <carsten_schmid@mentor.com>
Tested-by: Carsten Schmid <carsten_schmid@mentor.com>
---
Rationale:
- Changed title according to reduced functionality.
  Original title:
  kernel/resource.c: invalidate parent when freed resource has childs
- Added more information about why issuing this patch
---
 kernel/resource.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index c3cc6d85ec52..0db374029627 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1172,7 +1172,19 @@ EXPORT_SYMBOL(__request_region);
  * @n: resource region size
  *
  * The described resource region must match a currently busy region.
+ * If the region has children warn.
  */
+static void check_children(struct resource *parent)
+{
+	write_lock(&resource_lock);
+
+	if (parent->child)
+		WARN_ONCE(1, "%s: %s still has children, at least %s\n",
+				__func__, parent->name, parent->child->name);
+
+	write_unlock(&resource_lock);
+}
+
 void __release_region(struct resource *parent, resource_size_t start,
 		      resource_size_t n)
 {
@@ -1200,6 +1212,10 @@ void __release_region(struct resource *parent, resource_size_t start,
 			write_unlock(&resource_lock);
 			if (res->flags & IORESOURCE_MUXED)
 				wake_up(&muxed_resource_wait);
+
+			/* You should'nt release a resource that has children */
+			check_children(res);
+
 			free_resource(res);
 			return;
 		}
-- 
2.17.1