lkml.org 
[lkml]   [2019]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: Resend [PATCH] kernel/resource.c: invalidate parent when freed resource has childs
    From
    Date
    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);


    \
     
     \ /
      Last update: 2019-08-09 20:57    [W:4.245 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site