Message-ID: <aItRhGyTWNCJmXFA@pc>
Date: Thu, 31 Jul 2025 12:20:36 +0100
From: Salah Triki <salah.triki@...il.com>
To: Greg KH <gregkh@...uxfoundation.org>
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

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.

> > 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.

Regards,

Salah Triki

Powered by blists - more mailing lists