Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: "Schmid, Carsten" <Carsten_Schmid@mentor.com>
To: Hans de Goede <hdegoede@redhat.com>,
	Greg KH <gregkh@linuxfoundation.org>
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: AW: AW: KASAN: use-after-free Read in usbhid_power
Date: Fri, 9 Aug 2019 12:38:35 +0000
Message-ID: <932dbc769a80416eb736e6397c126ce9@SVR-IES-MBX-03.mgc.mentorg.com> (raw)
In-Reply-To: <9c955960-8b50-30dd-732a-92c62e5761cc@redhat.com>

Hi again,

>>
>> Hey, i did not want to trigger an eartquake in the basement of the kernel ;-)
>> My intention was to prevent some crashes, and help developers to find their bugs.
>> I think my patch exactly does this.
> 
> Hehe, actually drivers not being able to block unbind has been bugging me
> for
> a while now, because there are cases where this would be really helpful.
>>> 1) make resources refcounted, have child resources take a ref on the parent
>>> 2) Disallow unbind on devices with bound child-devices?
>>>
>> Exactly what i was thinking of in first attempts.
>> But i fear that would break even more use cases.
>>
>> Hans, directly regarding the driver:
>> The problem i see is that the xhci_intel_unregister_pdev which is added
>> as an action with devm_add_action_or_reset() is called late by the framework,
>> later than the usb_hcd_pci_remove() in xhci_pci_remove.
>> Is there any chance to trigger this before?
>> This is what Greg meant with "right order".
> 
> Ah, I missed that part, sure that should be easy, just stop using
> devm_add_action_or_reset() and do the xhci_intel_unregister_pdev()
> manually at the right time. The downside of this is that you also
> need to make sure it happens at the right time from probe error-paths
> but given the bug you are hitting, I guess that is probably
> already a problem.
> 
@Hans:
Sure, will have a look at this. I think i have found where to do that,
but need to check how to get the pdev pointer there ....

@Greg:
I am still confident that my patch in __release_region should be taken in.

Situation now without my patch:
If we have a device driver (or whatever) releasing a resource, the owner of
the child will have no notification that the parent is gone.
Accessing the parent (at least this will happen when trying to free the resource)
might have changed memory at the parent location, and what happens might
be an access to unmapped memory, whatever - an oops and we don't know why.
That's what i experienced and hunting.

Situation with the patch applied:
The owner gets a notification (parent=NULL) and we have an indication in
the kernel log.
If an owner of the resource where the parent is gone checks for the parent,
we are fine. If he doesn't check: we have a NULL pointer deref with a warning
message pointing to the root cause.

Isn't it better to have a pointer to a crash rather than having unreliable
racy crashes in such a case?

Have a nice weekend.

Best regards
Carsten

  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                 ` AW: " Schmid, Carsten
2019-08-09 10:20                   ` Hans de Goede
2019-08-09 10:47                     ` AW: " Schmid, Carsten
2019-08-09 10:53                       ` Hans de Goede
2019-08-09 12:38                         ` Schmid, Carsten [this message]
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=932dbc769a80416eb736e6397c126ce9@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