Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: Oliver Neukum <oneukum@suse.com>
Cc: andreyknvl@google.com, <syzkaller-bugs@googlegroups.com>,
	syzbot <syzbot+ef5de9c4f99c4edb4e49@syzkaller.appspotmail.com>,
	<linux-usb@vger.kernel.org>
Subject: Re: KASAN: use-after-free Read in usbhid_power
Date: Thu, 25 Jul 2019 11:09:24 -0400 (EDT)
Message-ID: <Pine.LNX.4.44L0.1907251057110.1512-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <1564047229.4670.14.camel@suse.com>

On Thu, 25 Jul 2019, Oliver Neukum wrote:

> Am Mittwoch, den 24.07.2019, 17:02 -0400 schrieb Alan Stern:
> > On Wed, 24 Jul 2019, Oliver Neukum wrote:
> > 
> > >  drivers/hid/usbhid/hid-core.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > > index c7bc9db5b192..98b996ecf4d3 100644
> > > --- a/drivers/hid/usbhid/hid-core.c
> > > +++ b/drivers/hid/usbhid/hid-core.c
> > > @@ -1229,6 +1229,17 @@ static int usbhid_power(struct hid_device *hid, int lvl)
> > >  	struct usbhid_device *usbhid = hid->driver_data;
> > >  	int r = 0;
> > >  
> > > +	spin_lock_irq(&usbhid->lock);
> > > +	if (test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
> > > +		r = -ENODEV;
> > > +		spin_unlock_irq(&usbhid->lock);
> > > +		goto bail_out;
> > > +	} else {
> > > +		/* protect against disconnect */
> > > +		usb_get_dev(interface_to_usbdev(usbhid->intf));
> > > +		spin_unlock_irq(&usbhid->lock);
> > > +	}
> > > +
> > >  	switch (lvl) {
> > >  	case PM_HINT_FULLON:
> > >  		r = usb_autopm_get_interface(usbhid->intf);
> > > @@ -1238,7 +1249,9 @@ static int usbhid_power(struct hid_device *hid, int lvl)
> > >  		usb_autopm_put_interface(usbhid->intf);
> > >  		break;
> > >  	}
> > > +	usb_put_dev(interface_to_usbdev(usbhid->intf));
> > >  
> > > +bail_out:
> > >  	return r;
> > >  }
> > 
> > Isn't this treating the symptom instead of the cause?
> 
> Sort of. Holding a reference for the whole time would have merit,
> but I doubt it is strictly necessary.

Just to be crystal clear, I was talking about a device reference --
usb_{get,put}_dev or usb_{get,put}_intf -- not a runtime PM reference.  

(Incidentally, your patch could be simplified by using usb_get_intf
instead of usb_get_dev.)

> > Shouldn't the hid_device hold a reference to usbhid->intf throughout 
> > its lifetime?  That way this sort of problem wouldn't arise in any 
> > routine, not just usbhid_power().
> 
> Unfortunately the semantics would still be wrong without the check
> in corner cases. In case disconnect() is called without a physical
> unplug, we must not touch the power state.
> I am indeed afraid that in that case my putative fix is still racy.
> But I don't to just introduce a mutex just for this. Any ideas?

That's a separate issue.  USB drivers -- indeed, all drivers -- are 
required to balance their runtime PM gets and puts (although in the 
case of a physical disconnection it doesn't matter).  Are you asking 
about the best way to do this?

Normally a driver's release or disconnect routine will stop all
asynchronous accesses to the device (interrupt handlers, work queues,
URBs, and so on).  At that point the only remaining runtime PM activity
will be whatever the routine itself does.  So it can see if any extra
runtime PM gets or puts are needed, and do whatever is necessary.

Does that answer your question?  I can't tell for sure...

Note: I did not try to track down the reason for the invalid access 
reported by syzbot.  It looked like a simple use-after-free, which 
would normally be fixed by taking the appropriate reference.  Which is 
what your patch does, except that it holds the reference only for a 
short time instead of over the entire lifetime of the private data 
structure (the usbhid structure), which is what normally happens.

Alan Stern


  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 [this message]
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                         ` 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=Pine.LNX.4.44L0.1907251057110.1512-100000@iolanthe.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=andreyknvl@google.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=oneukum@suse.com \
    --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 linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


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