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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 12 Aug 2019 08:39:10 +0000
From:   "Schmid, Carsten" <Carsten_Schmid@...tor.com>
To:     Wei Yang <richard.weiyang@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
CC:     "bp@...e.de" <bp@...e.de>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "osalvador@...e.de" <osalvador@...e.de>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "richardw.yang@...ux.intel.com" <richardw.yang@...ux.intel.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: AW: Resend [PATCH] kernel/resource.c: invalidate parent when freed
 resource has childs

> -----Ursprüngliche Nachricht-----
> Von: Wei Yang [mailto:richard.weiyang@...il.com]
> Gesendet: Samstag, 10. August 2019 02:45
> An: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Wei Yang <richard.weiyang@...il.com>; Schmid, Carsten
> <Carsten_Schmid@...tor.com>; bp@...e.de; dan.j.williams@...el.com;
> mingo@...nel.org; dave.hansen@...ux.intel.com; linux-
> kernel@...r.kernel.org; bhelgaas@...gle.com; osalvador@...e.de;
> rdunlap@...radead.org; richardw.yang@...ux.intel.com;
> gregkh@...uxfoundation.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@...il.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