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).
2. A `memory leak` if `udev`'s reference count is not properly
   decremented during driver disconnect, preventing the `usb_device`
   object from being freed.

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.

Signed-off-by: Salah Triki <salah.triki@gmail.com>
Fixes: 9c724357f432d ("[Bluetooth] Code cleanup of the drivers source code")
Cc: stable@vger.kernel.org
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes in v3:
    - Add tag Cc

Changes in v2:
    - Add tags Fixes and Cc
 drivers/bluetooth/bfusb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/bfusb.c b/drivers/bluetooth/bfusb.c
index 8df310983bf6..f966bd8361b0 100644
--- a/drivers/bluetooth/bfusb.c
+++ b/drivers/bluetooth/bfusb.c
@@ -622,7 +622,7 @@ static int bfusb_probe(struct usb_interface *intf, const struct usb_device_id *i
 	if (!data)
 		return -ENOMEM;
 
-	data->udev = udev;
+	data->udev = usb_get_dev(udev);
 	data->bulk_in_ep    = bulk_in_ep->desc.bEndpointAddress;
 	data->bulk_out_ep   = bulk_out_ep->desc.bEndpointAddress;
 	data->bulk_pkt_size = le16_to_cpu(bulk_out_ep->desc.wMaxPacketSize);
@@ -701,6 +701,8 @@ static void bfusb_disconnect(struct usb_interface *intf)
 
 	usb_set_intfdata(intf, NULL);
 
+	usb_put_dev(data->udev);
+
 	bfusb_close(hdev);
 
 	hci_unregister_dev(hdev);
-- 
2.43.0

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.

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

thanks,

greg k-h

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

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