Messages in this thread | | | Date | Mon, 7 Nov 2022 22:25:40 +0900 | From | Masami Hiramatsu (Google) <> | Subject | Re: [PATCH 1/2] tracing: kprobe: Fix potential null-ptr-deref on trace_event_file in kprobe_event_gen_test_exit() |
| |
On Mon, 7 Nov 2022 15:16:16 +0800 Shang XiaoJing <shangxiaojing@huawei.com> wrote:
> When trace_get_event_file() failed, gen_kretprobe_test will be assigned > as the error code. If module kprobe_event_gen_test is removed now, the > null pointer dereference will happen in kprobe_event_gen_test_exit(). > Check if gen_kprobe_test or gen_kretprobe_test is error code or NULL > before dereference them. > > BUG: kernel NULL pointer dereference, address: 0000000000000012 > PGD 0 P4D 0 > Oops: 0000 [#1] SMP PTI > CPU: 3 PID: 2210 Comm: modprobe Not tainted > 6.1.0-rc1-00171-g2159299a3b74-dirty #217 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014 > RIP: 0010:kprobe_event_gen_test_exit+0x1c/0xb5 [kprobe_event_gen_test] > Code: Unable to access opcode bytes at 0xffffffff9ffffff2. > RSP: 0018:ffffc900015bfeb8 EFLAGS: 00010246 > RAX: ffffffffffffffea RBX: ffffffffa0002080 RCX: 0000000000000000 > RDX: ffffffffa0001054 RSI: ffffffffa0001064 RDI: ffffffffdfc6349c > RBP: ffffffffa0000000 R08: 0000000000000004 R09: 00000000001e95c0 > R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000800 > R13: ffffffffa0002420 R14: 0000000000000000 R15: 0000000000000000 > FS: 00007f56b75be540(0000) GS:ffff88813bc00000(0000) > knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: ffffffff9ffffff2 CR3: 000000010874a006 CR4: 0000000000330ee0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > <TASK> > __x64_sys_delete_module+0x206/0x380 > ? lockdep_hardirqs_on_prepare+0xd8/0x190 > ? syscall_enter_from_user_mode+0x1c/0x50 > do_syscall_64+0x3f/0x90 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > Fixes: 64836248dda2 ("tracing: Add kprobe event command generation test module")
Ah, indeed. If anything wrong in the test code and if the gen_kprobe_test/gen_kretprobe_test is not initialized, this can happen.
Cc: stable@vger.kernel.org Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thank you!
> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> > --- > kernel/trace/kprobe_event_gen_test.c | 44 ++++++++++++++++++---------- > 1 file changed, 28 insertions(+), 16 deletions(-) > > diff --git a/kernel/trace/kprobe_event_gen_test.c b/kernel/trace/kprobe_event_gen_test.c > index d81f7c51025c..1c98fafcf333 100644 > --- a/kernel/trace/kprobe_event_gen_test.c > +++ b/kernel/trace/kprobe_event_gen_test.c > @@ -73,6 +73,10 @@ static struct trace_event_file *gen_kretprobe_test; > #define KPROBE_GEN_TEST_ARG3 NULL > #endif > > +static bool trace_event_file_is_valid(struct trace_event_file *input) > +{ > + return input && !IS_ERR(input); > +} > > /* > * Test to make sure we can create a kprobe event, then add more > @@ -217,10 +221,12 @@ static int __init kprobe_event_gen_test_init(void) > > ret = test_gen_kretprobe_cmd(); > if (ret) { > - WARN_ON(trace_array_set_clr_event(gen_kretprobe_test->tr, > - "kprobes", > - "gen_kretprobe_test", false)); > - trace_put_event_file(gen_kretprobe_test); > + if (trace_event_file_is_valid(gen_kretprobe_test)) { > + WARN_ON(trace_array_set_clr_event(gen_kretprobe_test->tr, > + "kprobes", > + "gen_kretprobe_test", false)); > + trace_put_event_file(gen_kretprobe_test); > + } > WARN_ON(kprobe_event_delete("gen_kretprobe_test")); > } > > @@ -229,24 +235,30 @@ static int __init kprobe_event_gen_test_init(void) > > static void __exit kprobe_event_gen_test_exit(void) > { > - /* Disable the event or you can't remove it */ > - WARN_ON(trace_array_set_clr_event(gen_kprobe_test->tr, > - "kprobes", > - "gen_kprobe_test", false)); > + if (trace_event_file_is_valid(gen_kprobe_test)) { > + /* Disable the event or you can't remove it */ > + WARN_ON(trace_array_set_clr_event(gen_kprobe_test->tr, > + "kprobes", > + "gen_kprobe_test", false)); > + > + /* Now give the file and instance back */ > + trace_put_event_file(gen_kprobe_test); > + } > > - /* Now give the file and instance back */ > - trace_put_event_file(gen_kprobe_test); > > /* Now unregister and free the event */ > WARN_ON(kprobe_event_delete("gen_kprobe_test")); > > - /* Disable the event or you can't remove it */ > - WARN_ON(trace_array_set_clr_event(gen_kretprobe_test->tr, > - "kprobes", > - "gen_kretprobe_test", false)); > + if (trace_event_file_is_valid(gen_kretprobe_test)) { > + /* Disable the event or you can't remove it */ > + WARN_ON(trace_array_set_clr_event(gen_kretprobe_test->tr, > + "kprobes", > + "gen_kretprobe_test", false)); > + > + /* Now give the file and instance back */ > + trace_put_event_file(gen_kretprobe_test); > + } > > - /* Now give the file and instance back */ > - trace_put_event_file(gen_kretprobe_test); > > /* Now unregister and free the event */ > WARN_ON(kprobe_event_delete("gen_kretprobe_test")); > -- > 2.17.1 >
-- Masami Hiramatsu (Google) <mhiramat@kernel.org>
| |