Re: [PATCH] HID: core: fix device cleanup on allocation failure

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

Pre-existing issues:
- [High] Concurrent `hidraw` operations can cause a use-after-free on the SRCU structure during device destruction because `hid_bpf_destroy_device()` runs before `hid_remove_device()`.
--

--- Patch [1]: [PATCH] HID: core: fix device cleanup on allocation failure ---
commit ef17fd6111206c1e7e801f388343922186fcf0dd
Author: Tristan Madani <[email protected]>

HID: core: fix device cleanup on allocation failure

This commit replaces hid_destroy_device() with put_device() in the error
path of hid_allocate_device(). This prevents a general protection fault
caused by cleaning up an uninitialized SRCU structure when 
hid_bpf_device_init() fails.

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index cf123347a2af7..9534de5317783 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -3060,7 +3060,12 @@ struct hid_device *hid_allocate_device(void)
>  	return hdev;
>  
>  out_err:
> -	hid_destroy_device(hdev);

[Severity: High]
This is a pre-existing issue, but looking at the implementation of 
hid_destroy_device(), it destroys the BPF structures before unregistering
the device:

void hid_destroy_device(struct hid_device *hdev)
{
	hid_bpf_destroy_device(hdev);
	hid_remove_device(hdev);
	put_device(&hdev->dev);
}

Can this order cause a use-after-free on the SRCU structure during a
concurrent device teardown? 

If a userspace process writes to the hidraw device concurrently with a 
hot-unplug, the hidraw writer could check the destroyed flag and then get 
preempted before taking the SRCU lock:

CPU1 (hidraw writer)
dispatch_hid_bpf_raw_requests()
	if (unlikely(hdev->bpf.destroyed))
		return -ENODEV;
	<preempted>

CPU2 (device unplug)
hid_destroy_device()
	hid_bpf_destroy_device()
		// sets bpf.destroyed = true
		cleanup_srcu_struct(&hdev->bpf.srcu);
	hid_remove_device() 
		// blocks waiting for CPU1

CPU1 (resumes)
	idx = srcu_read_lock(&hdev->bpf.srcu); 

Could CPU1 resume and dereference the freed SRCU internal pointers? 

Should hid_remove_device() be called before hid_bpf_destroy_device() to 
ensure userspace interfaces like hidraw are disconnected before the SRCU 
structure is destroyed?

> +	/*
> +	 * hid_destroy_device() cannot be used here because
> +	 * hid_bpf_device_init() failed and the SRCU struct was never
> +	 * initialized.  Release the device directly.
> +	 */
> +	put_device(&hdev->dev);
>  	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_GPL(hid_allocate_device);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1