[PATCH] HID: amd_sfh: clear hid_sensor_hubs entry on probe failure

[PATCH] HID: amd_sfh: clear hid_sensor_hubs entry on probe failure

[Chen Changcheng]

In amdtp_hid_probe(), the newly allocated HID device is stored in
cli_data->hid_sensor_hubs[cur_hid_dev] before calling hid_add_device().
If hid_add_device() fails, the error path frees the HID device and its
driver_data but does not clear the array entry, leaving a dangling
pointer.

When the caller (amd_sfh_hid_client_init or
amd_sfh1_1_hid_client_init) detects the probe failure, it jumps to its
cleanup label, which unconditionally calls amd_sfh_hid_client_deinit()
and subsequently amdtp_hid_remove(). The latter iterates over all
hid_sensor_hubs[] entries and, upon encountering the non-NULL but freed
pointer, performs a use-after-free read followed by double-free of both
the HID device and its driver_data.

Clear the array entry in the error path of amdtp_hid_probe() so that
amdtp_hid_remove() skips the failed entry.

Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
---
 drivers/hid/amd-sfh-hid/amd_sfh_hid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
index b04f675d49b0..9eaa2785a9ac 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
@@ -162,6 +162,7 @@ int amdtp_hid_probe(u32 cur_hid_dev, struct amdtp_cl_data *cli_data)
 	return 0;
 
 err_hid_device:
+	cli_data->hid_sensor_hubs[cur_hid_dev] = NULL;
 	kfree(hid_data);
 err_hid_data:
 	hid_destroy_device(hid);
-- 
2.25.1

Re: [PATCH] HID: amd_sfh: clear hid_sensor_hubs entry on probe failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Improper teardown ordering allows delayed work to be scheduled after cancellation, causing a Use-After-Free.
- [Medium] Unprocessed requests in `req_list` are leaked during driver teardown.
--

--- Patch [1]: [PATCH] HID: amd_sfh: clear hid_sensor_hubs entry on probe failure ---
commit 610c0ee48e98cb215b641a8b6eb6b1d6375db3cc
Author: Chen Changcheng <chenchangcheng@kylinos.cn>

HID: amd_sfh: clear hid_sensor_hubs entry on probe failure

This patch clears the hid_sensor_hubs entry in the error path of
amdtp_hid_probe(). Doing this prevents a dangling pointer that can
trigger a use-after-free and double-free during cleanup when
amdtp_hid_remove() iterates over the array.

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> index b04f675d49b0d..9eaa2785a9aca 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> @@ -162,6 +162,7 @@ int amdtp_hid_probe(u32 cur_hid_dev, struct amdtp_cl_data *cli_data)
>  	return 0;
>  
>  err_hid_device:
> +	cli_data->hid_sensor_hubs[cur_hid_dev] = NULL;
>  	kfree(hid_data);
>  err_hid_data:
>  	hid_destroy_device(hid);

[Severity: High]
This is a pre-existing issue, but is it possible for delayed work to be
scheduled after cancellation during driver teardown?

Looking at amd_sfh_hid_client_deinit() in
drivers/hid/amd-sfh-hid/amd_sfh_client.c:

	cancel_delayed_work_sync(&cl_data->work);
	cancel_delayed_work_sync(&cl_data->work_buffer);
	amdtp_hid_remove(cl_data);

Because amdtp_hid_remove() is called after the workqueues are canceled, the
HID interfaces remain fully operational during this window.

Could an unprivileged user concurrently issue a hidraw ioctl, which would call
amd_sfh_get_report() and re-arm the delayed work?

If this happens, the delayed work could run after the driver structures are
freed or destroyed, potentially leading to a use-after-free.

[Severity: Medium]
This is also a pre-existing issue, but does driver teardown leak unprocessed
request_list nodes?

In drivers/hid/amd-sfh-hid/amd_sfh_client.c, amd_sfh_get_report() dynamically
allocates new request_list nodes and appends them to cli_data->req_list:

			struct request_list *new = kzalloc_obj(*new);

			if (!new)
				return -ENOMEM;
...
			list_add(&new->list, &req_list->list);

These nodes are normally freed by the delayed work function amd_sfh_work().

However, when amd_sfh_hid_client_deinit() cancels the work, it does not appear
to iterate over req_list to free any pending requests. 

If there are unprocessed requests remaining in the list at the time of unbind,
will these dynamically allocated list nodes be permanently leaked?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715094622.115062-1-chenchangcheng@kylinos.cn?part=1