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
