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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).