[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 a3ff0514f9cdf..d8df23f67f1fd 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -3115,7 +3115,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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]

> 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:

Is the reasoning here fully accurate?

Looking at hid_bpf_device_init() in drivers/hid/bpf/hid_bpf_dispatch.c,
it returns the result of init_srcu_struct(&hdev->bpf.srcu), so
init_srcu_struct() is always called before the error path is taken.

When init_srcu_struct() fails (specifically init_srcu_struct_fields() in
kernel/rcu/srcutree.c), the failure paths explicitly free and NULL the
pointers:

kernel/rcu/srcutree.c:init_srcu_struct_fields() {
    ...
err_free_sda:
    free_percpu(ssp->sda);
    ssp->sda = NULL;
err_free_sup:
    kfree(ssp->srcu_sup);
    ssp->srcu_sup = NULL;
    ...
}

So the SRCU internal pointers are NULL due to init_srcu_struct()'s
rollback on failure, not because init_srcu_struct() was never called or
because they are still zeroed from kzalloc.

Could the commit message be more precise? Something like:
"init_srcu_struct() was called but failed, and it rolled the SRCU
internal pointers back to NULL"

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index a3ff0514f9cd..d8df23f67f1f 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -3115,7 +3115,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);
>  }

Should the comment say "was never successfully initialized" rather than
"was never initialized"?

As shown above, hid_bpf_device_init() returns init_srcu_struct()'s
result, so init_srcu_struct() is always called before reaching this
error path. It failed after partially initializing, then rolled back.

The distinction matters for future readers trying to understand the
ordering inside hid_bpf_device_init().


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33923149618