Linux USB
 help / color / mirror / Atom feed
From: "Danilo Krummrich" <dakr@kernel.org>
To: "Alan Stern" <stern@rowland.harvard.edu>
Cc: "Johan Hovold" <johan@kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Dmitry Torokhov" <dmitry.torokhov@gmail.com>,
	<driver-core@lists.linux.dev>, <linux-usb@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	"Mario Peter" <mario.peter@leica-geosystems.com>,
	<stable@vger.kernel.org>
Subject: Re: [PATCH] driver core: take parent lock on deferred probe
Date: Tue, 13 Oct 2026 23:01:16 +0200	[thread overview]
Message-ID: <DM40HVOBKMFI.3W1DK371ZWI80@kernel.org> (raw)
In-Reply-To: <ba24846a-7403-4bb1-a0cb-869cf1d44e05@rowland.harvard.edu>

On Tue Oct 13, 2026 at 9:11 PM CEST, Alan Stern wrote:
> On Tue, Oct 13, 2026 at 07:43:08PM +0200, Danilo Krummrich wrote:
>> On Tue Oct 13, 2026 at 4:06 PM CEST, Johan Hovold wrote:
>> > +void bus_probe_device_deferred(struct device *dev)
>> > +{
>> > +	if (dev->parent && dev->bus->need_parent_lock)
>> > +		device_lock(dev->parent);
>> 
>> This looks like it has the same potential UAF as the other patch.  It's quite
>> unfortunate we need the whole need_parent_lock thing just for USB.  IIUC the
>> main reason USB needs this is because interface devices share resources of USB
>> devices and we need to keep the locking order to avoid inversions, right?  Is
>> there more to this from the USB side of things?
>
> That's pretty much it.
>
>> I think we can probably do something like this
>> 
>> 	struct device *parent = NULL;
>> 
>> 	scoped_guard(device, dev) {
>> 		if (dev->p->dead)
>> 			return;
>> 	
>> 		if (dev->bus->need_parent_lock)
>> 			parent = get_device(dev->parent);
>> 	}
>> 
>> to avoid the problem, but it's still a bit ugly.
>> 
>> > +	__bus_probe_device(dev);
>> > +	if (dev->parent && dev->bus->need_parent_lock)
>> > +		device_unlock(dev->parent);
>> > +}
>
> Wouldn't it be simpler just to make each USB interface take a reference 
> to its parent device?  And likewise for USB devices, if needed?

The driver core already holds a reference count of the parent between
device_add() and device_del(), which is where a "parent" is technically defined.
But the deferred retry can extend beyond device_del().

USB interfaces already take an additional reference to their USB device in
usb_set_configuration() and drop it in usb_release_interface(), so they should
already be protected against this particular UAF.

The issue concerns a USB device and its parent hub's USB device.  (This makes me
wonder whether probing a USB device actually requires the parent USB HUB
device's lock to be held, or whether this is just a consequence of sharing the
same usb_bus_type?)

In theory a USB device could also keep a reference count to its parent, but then
the driver core code would rely on an implementation detail of its users; I'd
prefer not to make driver core's access to dev->parent depend on references
maintained by individual subsystems.

I'd even go a step further any say that if subsystems try to keep this pointer
valid beyond what's guaranteed by the driver core, they should do this with a
separate bus specific pointer.  The reason is that otherwise the lifetime and
ownership definition of this pointer does not depend on API contracts, but on
individual implementation details, which is a very common source for all kinds
of bugs.

      reply	other threads:[~2026-10-13 21:01 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-13 14:06 [PATCH] driver core: take parent lock on deferred probe Johan Hovold
2026-10-13 14:24 ` sashiko-bot
2026-10-13 17:43 ` Danilo Krummrich
2026-10-13 19:11   ` Alan Stern
2026-10-13 21:01     ` Danilo Krummrich [this message]

Reply instructions:

You may reply publicly 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=DM40HVOBKMFI.3W1DK371ZWI80@kernel.org \
    --to=dakr@kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=driver-core@lists.linux.dev \
    --cc=gregkh@linuxfoundation.org \
    --cc=johan@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mario.peter@leica-geosystems.com \
    --cc=rafael@kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=stern@rowland.harvard.edu \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox