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

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

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

hid_allocate_device() calls hid_destroy_device() in its error path when
hid_bpf_device_init() fails.  hid_destroy_device() in turn calls
hid_bpf_destroy_device() which invokes synchronize_srcu() and
cleanup_srcu_struct() on the SRCU structure.  However, at this point
init_srcu_struct() has not been called (or has failed), so the SRCU
internal pointers are all zeroed from kzalloc.  Dereferencing them
causes a general protection fault:

  general protection fault in check_init_srcu_struct
  KASAN: null-ptr-deref in range [0x00000000000001c0-0x00000000000001c7]
  Call Trace:
   synchronize_srcu+0x1c/0x2b0
   hid_bpf_destroy_device+0x64/0x90
   hid_destroy_device+0x29/0x240
   hid_allocate_device+0x420/0x4e0
   uhid_dev_create2+0x161/0x890

Fix this by replacing hid_destroy_device() with put_device() in the
error path.  At this point in hid_allocate_device(), the device has
been initialized (device_initialize) but not yet added (device_add),
so put_device() is the correct cleanup call.  It triggers the release
callback which frees the hid_device without attempting to clean up
resources that were never set up.

Found by syzkaller on Ubuntu 6.14.11 KASAN.

Fixes: 6cd735f0e57a ("HID: bpf: protect HID-BPF prog_list access by a SRCU")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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);
+	/*
+	 * 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);
-- 
2.47.3

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

[sashiko-bot]

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 <tristan@talencesecurity.com>

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/20260819013149.889913-1-tristmd@gmail.com?part=1