Hi Jinjie Ruan,
Thanks a lot for finding and fixing this bug.
On Thu, Sep 21, 2023 at 09:38:23PM +0800, Jinjie Ruan wrote:
> When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch kernel and
> then the below user-memory-access bug occurs.
>
> In hid_test_uclogic_params_cleanup_event_hooks(),it call
> uclogic_params_ugee_v2_init_event_hooks() with the first arg=NULL, so
> when it calls uclogic_params_ugee_v2_has_battery(), the hid_get_drvdata()
> will access hdev->dev with hdev=NULL, which will cause below
> user-memory-access.
>
> So add a fake_device with quirks member and call hid_set_drvdata()
> to assign hdev->dev->driver_data which avoids the null-ptr-def bug
> for drvdata->quirks in uclogic_params_ugee_v2_has_battery(). After applying
> this patch, the below user-memory-access bug never occurs.
>
> general protection fault, probably for non-canonical address 0xdffffc0000000329: 0000 [#1] PREEMPT SMP KASAN
> KASAN: probably user-memory-access in range [0x0000000000001948-0x000000000000194f]
> CPU: 5 PID: 2189 Comm: kunit_try_catch Tainted: G B W N 6.6.0-rc2+ #30
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
> Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
> RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
> RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
> RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
> RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
> R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
> R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
> FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
> DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
> DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> PKRU: 55555554
> Call Trace:
> <TASK>
> ? die_addr+0x3d/0xa0
> ? exc_general_protection+0x144/0x220
> ? asm_exc_general_protection+0x22/0x30
> ? uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
> ? sched_clock_cpu+0x69/0x550
> ? uclogic_parse_ugee_v2_desc_gen_params+0x70/0x70
> ? load_balance+0x2950/0x2950
> ? rcu_trc_cmpxchg_need_qs+0x67/0xa0
> hid_test_uclogic_params_cleanup_event_hooks+0x9e/0x1a0
> ? uclogic_params_ugee_v2_init_event_hooks+0x600/0x600
> ? __switch_to+0x5cf/0xe60
> ? migrate_enable+0x260/0x260
> ? __kthread_parkme+0x83/0x150
> ? kunit_try_run_case_cleanup+0xe0/0xe0
> kunit_generic_run_threadfn_adapter+0x4a/0x90
> ? kunit_try_catch_throw+0x80/0x80
> kthread+0x2b5/0x380
> ? kthread_complete_and_exit+0x20/0x20
> ret_from_fork+0x2d/0x70
> ? kthread_complete_and_exit+0x20/0x20
> ret_from_fork_asm+0x11/0x20
> </TASK>
> Modules linked in:
> Dumping ftrace buffer:
> (ftrace buffer empty)
> ---[ end trace 0000000000000000 ]---
> RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
> Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
> RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
> RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
> RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
> RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
> R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
> R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
> FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
> DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
> DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> PKRU: 55555554
> Kernel panic - not syncing: Fatal exception
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Kernel Offset: disabled
> Rebooting in 1 seconds..
>
> Fixes: a251d6576d2a ("HID: uclogic: Handle wireless device reconnection")
> Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
> ---
> drivers/hid/hid-uclogic-params-test.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-uclogic-params-test.c b/drivers/hid/hid-uclogic-params-test.c
> index 678f50cbb160..3938bae25982 100644
> --- a/drivers/hid/hid-uclogic-params-test.c
> +++ b/drivers/hid/hid-uclogic-params-test.c
> @@ -174,12 +174,22 @@ static void hid_test_uclogic_parse_ugee_v2_desc(struct kunit *test)
> KUNIT_EXPECT_EQ(test, params->frame_type, frame_type);
> }
>
> +struct fake_device {
> + unsigned long quirks;
> +};
> +
> static void hid_test_uclogic_params_cleanup_event_hooks(struct kunit *test)
> {
> int res, n;
> + struct hid_device *hdev;
> + struct fake_device *fake_dev;
> struct uclogic_params p = {0, };
>
> - res = uclogic_params_ugee_v2_init_event_hooks(NULL, &p);
> + hdev = kzalloc(sizeof(struct hid_device), GFP_KERNEL);
> + fake_dev = kzalloc(sizeof(struct fake_device), GFP_KERNEL);
Intead of using `kzalloc()` to allocate memory for `hdev` and `fake_dev`
we should use `kunit_kzalloc()`.
It has 2 main advatages:
- If an assertion fails, the memory is freed
- No need for `kfree()`
> + hid_set_drvdata(hdev, fake_dev);
> +
> + res = uclogic_params_ugee_v2_init_event_hooks(hdev, &p);
> KUNIT_ASSERT_EQ(test, res, 0);
>
> /* Check that the function can be called repeatedly */
> @@ -187,6 +197,9 @@ static void hid_test_uclogic_params_cleanup_event_hooks(struct kunit *test)
> uclogic_params_cleanup_event_hooks(&p);
> KUNIT_EXPECT_PTR_EQ(test, p.event_hooks, NULL);
> }
> +
> + kfree(fake_dev);
> + kfree(hdev);
This 2 lines can be removed if `kunit_kzalloc()` is used.
> }
>
> static struct kunit_case hid_uclogic_params_test_cases[] = {
> --
> 2.34.1
>
Once the `kunit_kzalloc()` change is appliyed:
Reviewed-by: José Expósito <jose.exposito89@xxxxxxxxx>
