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

[sashiko-bot@kernel.org]

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