Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: "Schmid, Carsten" <Carsten_Schmid@mentor.com>
To: Greg KH <gregkh@linuxfoundation.org>,
	"hdegoede@redhat.com" <hdegoede@redhat.com>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	Andrey Konovalov <andreyknvl@google.com>,
	Oliver Neukum <oneukum@suse.com>,
	syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
	syzbot <syzbot+ef5de9c4f99c4edb4e49@syzkaller.appspotmail.com>,
	USB list <linux-usb@vger.kernel.org>,
	Hillf Danton <hdanton@sina.com>
Subject: AW: KASAN: use-after-free Read in usbhid_power
Date: Fri, 9 Aug 2019 09:34:29 +0000
Message-ID: <8e43085507b849e49e858e5388f3e13c@SVR-IES-MBX-03.mgc.mentorg.com> (raw)
In-Reply-To: <20190809075555.GA20409@kroah.com>

> -----Ursprüngliche Nachricht-----
> Von: Greg KH [mailto:gregkh@linuxfoundation.org]
> Gesendet: Freitag, 9. August 2019 09:56
> An: Schmid, Carsten <Carsten_Schmid@mentor.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>; Andrey Konovalov
> <andreyknvl@google.com>; Oliver Neukum <oneukum@suse.com>;
> syzkaller-bugs <syzkaller-bugs@googlegroups.com>; syzbot
> <syzbot+ef5de9c4f99c4edb4e49@syzkaller.appspotmail.com>; USB list
> <linux-usb@vger.kernel.org>; Hillf Danton <hdanton@sina.com>
> Betreff: Re: KASAN: use-after-free Read in usbhid_power
> 
> On Fri, Aug 09, 2019 at 07:35:32AM +0000, Schmid, Carsten wrote:
> > Hi all having use-after-free issues in USB shutdowns:
> > I hunted for a similar case in the intel_xhci_usb_sw driver.
> > What i have found and proposed is (from yesterday):
> > ---
> > [PATCH] kernel/resource.c: invalidate parent when freed resource has
> childs
> >
> > 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);
> 
> Nit, should't this be written so that you only drop/get the lock if the
> above if statement is true?
> 
What if some other async part invalidates the child while accessing it?
I wanted to make sure that the res->child is valid through the time it is accessed.

> > +                       if (res->child) {
> > +                               printk(KERN_WARNING "__release_region: %s has child
> %s,"
> > +                                               "invalidating childs parent\n",
> > +                                               res->name, res->child->name);
> 
> What can userspace/anyone do about this if it triggers?
> 
At least a platform driver developer can see he did something wrong.
I had a look at the code of Hans and did not see anything weird.
(platform driver is in drivers/usb/host/xhci-ext-caps.c)
The issue is very racy - what happens when the platform code accesses the
resource mainly depends on if the freed resource memory already has been
reused by someone.

It was hard to find that, and only a single core dump enabled me to find it.
A first attempt was to set resource count to 0 in Hans' driver in the unregister
pdev before calling platform_device_unregister. That worked.
But this is a dirty hack in my eyes. The framework should detect such issues,
so i decided to find the best place where to insert the check - and
found it is the place where the resource is freed and still has childrens.

> Can't we fix the root problem in the xhci driver to properly order how
> it tears things down?  If it has intel_xhci_usb_driver as a "child"
> shouldn't that be unbound/freed before the parent is?
> 
Hans, any idea?

> thanks,
> 
> greg k-h

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-23 12:48 syzbot
2019-07-24 14:17 ` Oliver Neukum
2019-07-24 14:24   ` Andrey Konovalov
2019-07-24 20:55 ` Oliver Neukum
2019-07-24 21:02   ` Alan Stern
2019-07-25  9:33     ` Oliver Neukum
2019-07-25 15:09       ` Alan Stern
2019-08-08 18:54         ` Andrey Konovalov
2019-08-08 19:37           ` Alan Stern
2019-08-09  7:35             ` AW: " Schmid, Carsten
2019-08-09  7:55               ` Greg KH
2019-08-09  9:34                 ` Schmid, Carsten [this message]
2019-08-09 10:20                   ` AW: " Hans de Goede
2019-08-09 10:47                     ` AW: " Schmid, Carsten
2019-08-09 10:53                       ` Hans de Goede
2019-08-09 12:38                         ` AW: " Schmid, Carsten
2019-08-09 12:54                           ` Greg KH
2019-08-09 13:00                             ` AW: " Schmid, Carsten
2019-08-09 13:38                               ` Greg KH
2019-08-10 11:12                           ` AW: " Hans de Goede
2019-08-09 17:36             ` Andrey Konovalov
2019-08-09 18:12               ` syzbot
2019-08-12 14:29                 ` Andrey Konovalov
2019-08-09 20:26           ` Oliver Neukum
2019-07-24 21:16   ` syzbot
2019-07-25 11:22     ` Andrey Konovalov
2019-07-25 12:15 ` Oliver Neukum
2019-07-25 12:27   ` syzbot

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8e43085507b849e49e858e5388f3e13c@SVR-IES-MBX-03.mgc.mentorg.com \
    --to=carsten_schmid@mentor.com \
    --cc=andreyknvl@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdanton@sina.com \
    --cc=hdegoede@redhat.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=oneukum@suse.com \
    --cc=stern@rowland.harvard.edu \
    --cc=syzbot+ef5de9c4f99c4edb4e49@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git