On 2023/10/9 1:21, José Expósito wrote:
> 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()`
Thank you! I'll fix it sooner.
>
>> + 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>
>
