Message-ID: <2025073152-molecular-porthole-c949@gregkh>
Date: Thu, 31 Jul 2025 13:52:25 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Salah Triki <salah.triki@...il.com>
Cc: Markus Elfring <Markus.Elfring@....de>,
	Marcel Holtmann <marcel@...tmann.org>,
	Luiz Augusto von Dentz <luiz.dentz@...il.com>,
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH V3] Bluetooth: bfusb: Fix use-after-free and memory leak
 in device lifecycle

On Thu, Jul 31, 2025 at 12:20:36PM +0100, Salah Triki wrote:
> Hello Greg,
> 
> Thanks for your feedback.
> 
> On Thu, Jul 31, 2025 at 06:32:35AM +0200, Greg KH wrote:
> > On Thu, Jul 31, 2025 at 03:19:19AM +0100, Salah Triki wrote:
> > > The driver stores a reference to the `usb_device` structure (`udev`)
> > > in its private data (`data->udev`), which can persist beyond the
> > > immediate context of the `bfusb_probe()` function.
> > > 
> > > Without proper reference count management, this can lead to two issues:
> > > 
> > > 1. A `use-after-free` scenario if `udev` is accessed after its main
> > >    reference count drops to zero (e.g., if the device is disconnected
> > >    and the `data` structure is still active).
> > 
> > How can that happen as during the probe/remove cycle, the reference
> > count is always properly incremetned.
> > 
> > > 2. A `memory leak` if `udev`'s reference count is not properly
> > >    decremented during driver disconnect, preventing the `usb_device`
> > >    object from being freed.
> > 
> > There is no leak here at all, sorry.
> > 
> 
> I understand your concern about the existence of a memory leak or 
> use-after-free scenario in the driver's current context.
> 
> My intention with this patch is to ensure the driver adheres to best
> practices for managing `usb_device` structure references, as outlined in
> the kernel's documentation. The `usb_get_dev()` function is explicitly
> designed for use when a driver stores a reference to a `usb_device`
> structure in its private data, which is the case here with `data->udev`.
> 
> As the documentation for `usb_get_dev()` states:
> 
> ``Each live reference to a device should be refcounted. Drivers for USB
> interfaces should normally record such references in their probe()
> methods, when they bind to an interface, and release them by calling
> usb_put_dev(), in their disconnect() methods.``
> 
> By following this recommendation, adding `usb_get_dev(udev)` in
> `bfusb_probe()` and `usb_put_dev(data->udev)` in `bfusb_disconnect()`
> ensures the `udev` structure's lifetime is explicitly managed by the driver
> as long as it's being referenced. This proactively prevents potential
> issues that could arise in future scenarios, even if a specific problem
> hasn't been observed or reported yet.

Yes, I agree with the documentation, I wrote it :)

But, I am saying, you are NOT actually fixing anything here.  It's a
"best practice" but due to the fact that the dev pointer is only being
reference counted by your change across the probe/release function, it
is a pointless change.

It's also a "dangerous" change in that you are trying to say "this fixes
a security issue!" when it does not do anything like that at all.

> > > To correctly manage the `udev` lifetime, explicitly increment its
> > > reference count with `usb_get_dev(udev)` when storing it in the
> > > driver's private data. Correspondingly, decrement the reference count
> > > with `usb_put_dev(data->udev)` in the `bfusb_disconnect()` callback.
> > > 
> > > This ensures `udev` remains valid while referenced by the driver's
> > > private data and is properly released when no longer needed.
> > 
> > How was this tested?
> > 
> > I'm not saying the change is wrong, just that I don't think it's
> > actually a leak, or fix of anything real.
> > 
> > Or do you have a workload that shows this is needed?  If so, what is the
> > crash reported?
> > 
> 
> While I don't have a specific workload that reproduces a current crash or
> memory leak, this patch aims to enhance the driver's robustness by
> aligning its behavior with the established conventions for managing
> `usb_device` object references. It's a preventive measure to ensure the
> driver correctly handles the lifetime of the `usb_device` object it
> references, even in scenarios of unexpected disconnection or re-enumeration
> that might otherwise have unforeseen consequences.
> 
> Please let me know if you have any further questions.

Please test this to see if it actually makes any difference in the code
before making claims that it fixes a real bug.

thanks,

greg k-h

Powered by blists - more mailing lists