From: "José Expósito" <jose.exposito89@gmail.com>
To: Jinjie Ruan <ruanjinjie@huawei.com>
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-input@vger.kernel.org
Subject: Re: [PATCH 1/2] HID: uclogic: Fix user-memory-access bug in uclogic_params_ugee_v2_init_event_hooks()
Date: Sun, 8 Oct 2023 19:21:00 +0200 [thread overview]
Message-ID: <ZSLk/JfhtJOp6Z81@fedora> (raw)
In-Reply-To: <20230921133824.605700-2-ruanjinjie@huawei.com>
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@huawei.com>
> ---
> 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@gmail.com>
next prev parent reply other threads:[~2023-10-08 17:21 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-21 13:38 [PATCH 0/2] HID: uclogic: Fix two bugs in uclogic Jinjie Ruan
2023-09-21 13:38 ` [PATCH 1/2] HID: uclogic: Fix user-memory-access bug in uclogic_params_ugee_v2_init_event_hooks() Jinjie Ruan
2023-10-08 17:21 ` José Expósito [this message]
2023-10-09 2:19 ` Jinjie Ruan
2023-09-21 13:38 ` [PATCH 2/2] HID: uclogic: Fix a work->entry not empty bug in __queue_work() Jinjie Ruan
2023-10-08 17:21 ` José Expósito
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZSLk/JfhtJOp6Z81@fedora \
--to=jose.exposito89@gmail.com \
--cc=benjamin.tissoires@redhat.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=ruanjinjie@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.