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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, Aug 9, 2019 at 6:50 AM Schmid, Carsten
<[email protected]> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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);
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 <[email protected]>
---
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;
}
On Fri, Aug 9, 2019 at 6:50 AM Schmid, Carsten
<[email protected]> 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
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 <[email protected]>
>---
>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
On Fri, Aug 9, 2019 at 3:38 PM Wei Yang <[email protected]> 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
On Fri, Aug 09, 2019 at 03:45:59PM -0700, Linus Torvalds wrote:
>On Fri, Aug 9, 2019 at 3:38 PM Wei Yang <[email protected]> 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
> -----Urspr?ngliche Nachricht-----
> Von: Wei Yang [mailto:[email protected]]
> Gesendet: Samstag, 10. August 2019 02:45
> An: Linus Torvalds <[email protected]>
> Cc: Wei Yang <[email protected]>; Schmid, Carsten
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> 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
> >
> > 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
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 <[email protected]>
---
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
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 <[email protected]>
>---
>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
>>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
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