[PATCH] HID: Input: Add battery list cleanup in hidinput_disconnect()

[PATCH] HID: Input: Add battery list cleanup in hidinput_disconnect()

[Rafael Passos]

The batteries list (hdev->batteries) is not cleaned up during
hidinput_disconnect(), but struct hid_battery entries are allocated
with devm_kzalloc.
When a driver is unbound (e.g. during devicereprobe), devm frees those
entries while their list_head nodesremain dangling in hdev->batteries,
which persists across rebinds.

Fixes: 4a58ae85c3f9 ("HID: input: Add support for multiple batteries per device")
Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---

Hi, I hope there is time to land this in this cycle.
Tell me if I should make any changes.
I built and tested it againts the mainline.

Bug report link:
    https://lore.kernel.org/all/20260602011949.2825852-1-rafael@rcpassos.me/

 drivers/hid/hid-input.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index d73cfa2e73d3..95815e140fdf 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -2408,6 +2408,15 @@ EXPORT_SYMBOL_GPL(hidinput_connect);
 void hidinput_disconnect(struct hid_device *hid)
 {
 	struct hid_input *hidinput, *next;
+#ifdef CONFIG_HID_BATTERY_STRENGTH
+	{
+		struct hid_battery *bat, *tmp;
+
+		list_for_each_entry_safe(bat, tmp, &hid->batteries, list) {
+			list_del_init(&bat->list);
+		}
+	}
+#endif
 
 	list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
 		list_del(&hidinput->list);
-- 
2.53.0

Re: [PATCH] HID: Input: Add battery list cleanup in hidinput_disconnect()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch introduces a Use-After-Free (UAF) and a soft-lockup race condition by manually unlinking `devm`-managed battery structures from `hid->batteries` during `hidinput_disconnect()`.
--

commit f0760b781af9f4b5e629ebfa0c88970cbaaea8fe
Author: Rafael Passos <rafael@rcpassos.me>

HID: Input: Add battery list cleanup in hidinput_disconnect()

The batteries list (hdev->batteries) is not cleaned up during
hidinput_disconnect(), but struct hid_battery entries are allocated
with devm_kzalloc. When a driver is unbound, devm frees those entries
while their list_head nodes remain dangling in hdev->batteries.

> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index d73cfa2e73d33..95815e140fdf2 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -2408,6 +2408,15 @@ EXPORT_SYMBOL_GPL(hidinput_connect);
>  void hidinput_disconnect(struct hid_device *hid)
>  {
>  	struct hid_input *hidinput, *next;
> +#ifdef CONFIG_HID_BATTERY_STRENGTH
> +	{
> +		struct hid_battery *bat, *tmp;
> +
> +		list_for_each_entry_safe(bat, tmp, &hid->batteries, list) {
> +			list_del_init(&bat->list);

[Severity: High]
Does this introduce a use-after-free and a race condition?

If a driver releases its devres group before calling hid_hw_stop(), the
battery structures allocated with devm_kzalloc() will be freed early. For
example, wacom_mode_change_work() in drivers/hid/wacom_sys.c acts like
this:

wacom_mode_change_work()
   wacom_release_resources(wacom1);
   hid_hw_stop(wacom1->hdev);

Since wacom_release_resources() releases the devres group, the battery
memory is freed before hid_hw_stop() calls hidinput_disconnect(). When we
reach this loop, we will iterate over and modify already-freed memory.

Additionally, hid_hw_stop() can be called asynchronously without holding
hdev->driver_input_lock. Modifying hid->batteries here can race with
concurrent readers like hidinput_update_battery(), which iterates over
the list, potentially causing a crash or infinite loop.

Could the cleanup be handled by adding a custom action during battery setup
via devm_add_action_or_reset(), so it is guaranteed to happen exactly when
the devm resource is freed?

> +		}
> +	}
> +#endif

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602015029.2838058-1-rafael@rcpassos.me?part=1