Message ID | 20220923093253.177298-1-lihuafei1@huawei.com (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | powerpc/kprobes: Fix null pointer reference in arch_prepare_kprobe() | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Li Huafei wrote: > I found a null pointer reference in arch_prepare_kprobe(): Good find! > > # echo 'p cmdline_proc_show' > kprobe_events > # echo 'p cmdline_proc_show+16' >> kprobe_events I think we should extend multiple_kprobes selftest to also place contiguous probes to catch such errors. > [ 67.278533][ T122] Kernel attempted to read user page (0) - exploit attempt? (uid: 0) > [ 67.279326][ T122] BUG: Kernel NULL pointer dereference on read at 0x00000000 > [ 67.279738][ T122] Faulting instruction address: 0xc000000000050bfc > [ 67.280486][ T122] Oops: Kernel access of bad area, sig: 11 [#1] > [ 67.280846][ T122] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV > [ 67.281435][ T122] Modules linked in: > [ 67.281903][ T122] CPU: 0 PID: 122 Comm: sh Not tainted 6.0.0-rc3-00007-gdcf8e5633e2e #10 > [ 67.282547][ T122] NIP: c000000000050bfc LR: c000000000050bec CTR: 0000000000005bdc > [ 67.282920][ T122] REGS: c0000000348475b0 TRAP: 0300 Not tainted (6.0.0-rc3-00007-gdcf8e5633e2e) > [ 67.283424][ T122] MSR: 9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 88002444 XER: 20040006 > [ 67.284023][ T122] CFAR: c00000000022d100 DAR: 0000000000000000 DSISR: 40000000 IRQMASK: 0 > [ 67.284023][ T122] GPR00: c000000000050bec c000000034847850 c0000000013f6100 c000000001fb7718 > [ 67.284023][ T122] GPR04: c000000000515c10 c000000000e5fe08 c00000000133da60 c000000004839300 > [ 67.284023][ T122] GPR08: c0000000014ffb98 0000000000000000 c000000000515c0c c000000000e18576 > [ 67.284023][ T122] GPR12: c000000000e60170 c0000000015a0000 00000001155e0460 0000000000000000 > [ 67.284023][ T122] GPR16: 0000000000000000 00007fffe8eeb3c8 0000000116320728 0000000000000000 > [ 67.284023][ T122] GPR20: 0000000116320720 0000000000000000 c0000000012fa918 0000000000000006 > [ 67.284023][ T122] GPR24: c0000000014ffb98 c0000000011ed360 0000000000000000 c000000001fb7928 > [ 67.284023][ T122] GPR28: 0000000000000000 0000000000000000 000000007c0802a6 c000000001fb7918 > [ 67.287799][ T122] NIP [c000000000050bfc] arch_prepare_kprobe+0x10c/0x2d0 > [ 67.288490][ T122] LR [c000000000050bec] arch_prepare_kprobe+0xfc/0x2d0 > [ 67.289025][ T122] Call Trace: > [ 67.289268][ T122] [c000000034847850] [c0000000012f77a0] 0xc0000000012f77a0 (unreliable) > [ 67.289999][ T122] [c0000000348478d0] [c000000000231320] register_kprobe+0x3c0/0x7a0 > [ 67.290439][ T122] [c000000034847940] [c0000000002938c0] __register_trace_kprobe+0x140/0x1a0 > [ 67.290898][ T122] [c0000000348479b0] [c0000000002944c4] __trace_kprobe_create+0x794/0x1040 > [ 67.291330][ T122] [c000000034847b60] [c0000000002a1614] trace_probe_create+0xc4/0xe0 > [ 67.291717][ T122] [c000000034847bb0] [c00000000029363c] create_or_delete_trace_kprobe+0x2c/0x80 > [ 67.292158][ T122] [c000000034847bd0] [c000000000264420] trace_parse_run_command+0xf0/0x210 > [ 67.292611][ T122] [c000000034847c70] [c0000000002934a0] probes_write+0x20/0x40 > [ 67.292996][ T122] [c000000034847c90] [c00000000045e98c] vfs_write+0xfc/0x450 > [ 67.293356][ T122] [c000000034847d50] [c00000000045eec4] ksys_write+0x84/0x140 > [ 67.293716][ T122] [c000000034847da0] [c00000000002e4fc] system_call_exception+0x17c/0x3a0 > [ 67.294186][ T122] [c000000034847e10] [c00000000000c0e8] system_call_vectored_common+0xe8/0x278 > [ 67.294680][ T122] --- interrupt: 3000 at 0x7fffa5682de0 > [ 67.294937][ T122] NIP: 00007fffa5682de0 LR: 0000000000000000 CTR: 0000000000000000 > [ 67.295313][ T122] REGS: c000000034847e80 TRAP: 3000 Not tainted (6.0.0-rc3-00007-gdcf8e5633e2e) > [ 67.295725][ T122] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 44002408 XER: 00000000 > [ 67.296291][ T122] IRQMASK: 0 > [ 67.296291][ T122] GPR00: 0000000000000004 00007fffe8eeaec0 00007fffa5757300 0000000000000001 > [ 67.296291][ T122] GPR04: 0000000116329c60 0000000000000017 0000000000116329 0000000000000000 > [ 67.296291][ T122] GPR08: 0000000000000006 0000000000000000 0000000000000000 0000000000000000 > [ 67.296291][ T122] GPR12: 0000000000000000 00007fffa580ac60 00000001155e0460 0000000000000000 > [ 67.296291][ T122] GPR16: 0000000000000000 00007fffe8eeb3c8 0000000116320728 0000000000000000 > [ 67.296291][ T122] GPR20: 0000000116320720 0000000000000000 0000000000000000 0000000000000002 > [ 67.296291][ T122] GPR24: 00000001163206f0 0000000000000020 00007fffe8eeafa0 0000000000000001 > [ 67.296291][ T122] GPR28: 0000000000000000 0000000000000017 0000000116329c60 0000000000000001 > [ 67.299570][ T122] NIP [00007fffa5682de0] 0x7fffa5682de0 > [ 67.299837][ T122] LR [0000000000000000] 0x0 > [ 67.300072][ T122] --- interrupt: 3000 > [ 67.300447][ T122] Instruction dump: > [ 67.300736][ T122] 386319d8 481342f5 60000000 60000000 60000000 e87f0028 3863fffc 481dc4d1 > [ 67.301230][ T122] 60000000 2c230000 41820018 e9230058 <81290000> 552936be 2c090001 4182018c > [ 67.302102][ T122] ---[ end trace 0000000000000000 ]--- > [ 67.302496][ T122] Please consider trimming the backtrace to only capture the necessary information: https://docs.kernel.org/process/submitting-patches.html#backtraces-in-commit-mesages > > The address being probed has some special: > > cmdline_proc_show: Probe based on ftrace > cmdline_proc_show+16: Probe for the next instruction at the ftrace location > > The ftrace-based kprobe does not generate kprobe::ainsn::insn, it gets > set to NULL. In arch_prepare_kprobe() it will check for: > > ... > prev = get_kprobe(p->addr - 1); > preempt_enable_no_resched(); > if (prev && ppc_inst_prefixed(ppc_inst_read(prev->ainsn.insn))) { > ... > > If prev is based on ftrace, 'ppc_inst_read(prev->ainsn.insn)' will occur > with a null pointer reference. At this point prev->addr will not be a > prefixed instruction, so the check can be skipped. > > Check if prev is ftrace-based kprobe before reading 'prev->ainsn.insn' > to fix this problem. > > Fixes: b4657f7650ba ("powerpc/kprobes: Don't allow breakpoints on suffixes") > Signed-off-by: Li Huafei <lihuafei1@huawei.com> > --- > arch/powerpc/kernel/kprobes.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c > index 912d4f8a13be..9f6cbbd56809 100644 > --- a/arch/powerpc/kernel/kprobes.c > +++ b/arch/powerpc/kernel/kprobes.c > @@ -161,7 +161,12 @@ int arch_prepare_kprobe(struct kprobe *p) > preempt_disable(); > prev = get_kprobe(p->addr - 1); > preempt_enable_no_resched(); > - if (prev && ppc_inst_prefixed(ppc_inst_read(prev->ainsn.insn))) { > + /* > + * When prev is a ftrace-based kprobe, we don't have an insn, and it > + * doesn't probe for prefixed instruction. > + */ > + if (prev && !kprobe_ftrace(prev) && > + ppc_inst_prefixed(ppc_inst_read(prev->ainsn.insn))) { > printk("Cannot register a kprobe on the second word of prefixed instruction\n"); > ret = -EINVAL; > } It's fine to keep the if condition on a single line. Other than that, thanks for the fix! Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> - Naveen
On Fri, 23 Sep 2022 17:32:53 +0800, Li Huafei wrote: > I found a null pointer reference in arch_prepare_kprobe(): > > # echo 'p cmdline_proc_show' > kprobe_events > # echo 'p cmdline_proc_show+16' >> kprobe_events > [ 67.278533][ T122] Kernel attempted to read user page (0) - exploit attempt? (uid: 0) > [ 67.279326][ T122] BUG: Kernel NULL pointer dereference on read at 0x00000000 > [ 67.279738][ T122] Faulting instruction address: 0xc000000000050bfc > [ 67.280486][ T122] Oops: Kernel access of bad area, sig: 11 [#1] > [ 67.280846][ T122] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV > [ 67.281435][ T122] Modules linked in: > [ 67.281903][ T122] CPU: 0 PID: 122 Comm: sh Not tainted 6.0.0-rc3-00007-gdcf8e5633e2e #10 > [ 67.282547][ T122] NIP: c000000000050bfc LR: c000000000050bec CTR: 0000000000005bdc > [ 67.282920][ T122] REGS: c0000000348475b0 TRAP: 0300 Not tainted (6.0.0-rc3-00007-gdcf8e5633e2e) > [ 67.283424][ T122] MSR: 9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 88002444 XER: 20040006 > [ 67.284023][ T122] CFAR: c00000000022d100 DAR: 0000000000000000 DSISR: 40000000 IRQMASK: 0 > [ 67.284023][ T122] GPR00: c000000000050bec c000000034847850 c0000000013f6100 c000000001fb7718 > [ 67.284023][ T122] GPR04: c000000000515c10 c000000000e5fe08 c00000000133da60 c000000004839300 > [ 67.284023][ T122] GPR08: c0000000014ffb98 0000000000000000 c000000000515c0c c000000000e18576 > [ 67.284023][ T122] GPR12: c000000000e60170 c0000000015a0000 00000001155e0460 0000000000000000 > [ 67.284023][ T122] GPR16: 0000000000000000 00007fffe8eeb3c8 0000000116320728 0000000000000000 > [ 67.284023][ T122] GPR20: 0000000116320720 0000000000000000 c0000000012fa918 0000000000000006 > [ 67.284023][ T122] GPR24: c0000000014ffb98 c0000000011ed360 0000000000000000 c000000001fb7928 > [ 67.284023][ T122] GPR28: 0000000000000000 0000000000000000 000000007c0802a6 c000000001fb7918 > [ 67.287799][ T122] NIP [c000000000050bfc] arch_prepare_kprobe+0x10c/0x2d0 > [ 67.288490][ T122] LR [c000000000050bec] arch_prepare_kprobe+0xfc/0x2d0 > [ 67.289025][ T122] Call Trace: > [ 67.289268][ T122] [c000000034847850] [c0000000012f77a0] 0xc0000000012f77a0 (unreliable) > [ 67.289999][ T122] [c0000000348478d0] [c000000000231320] register_kprobe+0x3c0/0x7a0 > [ 67.290439][ T122] [c000000034847940] [c0000000002938c0] __register_trace_kprobe+0x140/0x1a0 > [ 67.290898][ T122] [c0000000348479b0] [c0000000002944c4] __trace_kprobe_create+0x794/0x1040 > [ 67.291330][ T122] [c000000034847b60] [c0000000002a1614] trace_probe_create+0xc4/0xe0 > [ 67.291717][ T122] [c000000034847bb0] [c00000000029363c] create_or_delete_trace_kprobe+0x2c/0x80 > [ 67.292158][ T122] [c000000034847bd0] [c000000000264420] trace_parse_run_command+0xf0/0x210 > [ 67.292611][ T122] [c000000034847c70] [c0000000002934a0] probes_write+0x20/0x40 > [ 67.292996][ T122] [c000000034847c90] [c00000000045e98c] vfs_write+0xfc/0x450 > [ 67.293356][ T122] [c000000034847d50] [c00000000045eec4] ksys_write+0x84/0x140 > [ 67.293716][ T122] [c000000034847da0] [c00000000002e4fc] system_call_exception+0x17c/0x3a0 > [ 67.294186][ T122] [c000000034847e10] [c00000000000c0e8] system_call_vectored_common+0xe8/0x278 > [ 67.294680][ T122] --- interrupt: 3000 at 0x7fffa5682de0 > [ 67.294937][ T122] NIP: 00007fffa5682de0 LR: 0000000000000000 CTR: 0000000000000000 > [ 67.295313][ T122] REGS: c000000034847e80 TRAP: 3000 Not tainted (6.0.0-rc3-00007-gdcf8e5633e2e) > [ 67.295725][ T122] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 44002408 XER: 00000000 > [ 67.296291][ T122] IRQMASK: 0 > [ 67.296291][ T122] GPR00: 0000000000000004 00007fffe8eeaec0 00007fffa5757300 0000000000000001 > [ 67.296291][ T122] GPR04: 0000000116329c60 0000000000000017 0000000000116329 0000000000000000 > [ 67.296291][ T122] GPR08: 0000000000000006 0000000000000000 0000000000000000 0000000000000000 > [ 67.296291][ T122] GPR12: 0000000000000000 00007fffa580ac60 00000001155e0460 0000000000000000 > [ 67.296291][ T122] GPR16: 0000000000000000 00007fffe8eeb3c8 0000000116320728 0000000000000000 > [ 67.296291][ T122] GPR20: 0000000116320720 0000000000000000 0000000000000000 0000000000000002 > [ 67.296291][ T122] GPR24: 00000001163206f0 0000000000000020 00007fffe8eeafa0 0000000000000001 > [ 67.296291][ T122] GPR28: 0000000000000000 0000000000000017 0000000116329c60 0000000000000001 > [ 67.299570][ T122] NIP [00007fffa5682de0] 0x7fffa5682de0 > [ 67.299837][ T122] LR [0000000000000000] 0x0 > [ 67.300072][ T122] --- interrupt: 3000 > [ 67.300447][ T122] Instruction dump: > [ 67.300736][ T122] 386319d8 481342f5 60000000 60000000 60000000 e87f0028 3863fffc 481dc4d1 > [ 67.301230][ T122] 60000000 2c230000 41820018 e9230058 <81290000> 552936be 2c090001 4182018c > [ 67.302102][ T122] ---[ end trace 0000000000000000 ]--- > [ 67.302496][ T122] > > [...] Applied to powerpc/next. [1/1] powerpc/kprobes: Fix null pointer reference in arch_prepare_kprobe() https://git.kernel.org/powerpc/c/97f88a3d723162781d6cbfdc7b9617eefab55b19 cheers
On 2022/9/30 17:47, Naveen N. Rao wrote: > Li Huafei wrote: >> I found a null pointer reference in arch_prepare_kprobe(): > > Good find! > Hi Naveen, Thank you for the review. >> >> # echo 'p cmdline_proc_show' > kprobe_events >> # echo 'p cmdline_proc_show+16' >> kprobe_events > > I think we should extend multiple_kprobes selftest to also place > contiguous probes to catch such errors. > Yes. But each architecture implementation is different and it looks a little difficult to decide which offsets need to be tested. >> [ 67.278533][ T122] Kernel attempted to read user page (0) - >> exploitattempt? (uid: 0) >> [ 67.279326][ T122] BUG: Kernel NULL pointer dereference on read >> at 0x00000000 >> [ 67.279738][ T122] Faulting instruction address: 0xc000000000050bfc >> [ 67.280486][ T122] Oops: Kernel access of bad area, sig: 11 [#1] >> [ 67.280846][ T122] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 >> NUMA PowerNV >> [ 67.281435][ T122] Modules linked in: >> [ 67.281903][ T122] CPU: 0 PID: 122 Comm: sh Not tainted >> 6.0.0-rc3-00007-gdcf8e5633e2e #10 >> [ 67.282547][ T122] NIP: c000000000050bfc LR: c000000000050bec >> CTR:0000000000005bdc >> [ 67.282920][ T122] REGS: c0000000348475b0 TRAP: 0300 Not >> tainted (6.0.0-rc3-00007-gdcf8e5633e2e) >> [ 67.283424][ T122] MSR: 9000000000009033 >> <SF,HV,EE,ME,IR,DR,RI,LE> CR: 88002444 XER: 20040006 >> [ 67.284023][ T122] CFAR: c00000000022d100 DAR: 0000000000000000 >> DSISR: 40000000 IRQMASK: 0 >> [ 67.284023][ T122] GPR00: c000000000050bec c000000034847850 >> c0000000013f6100 c000000001fb7718 >> [ 67.284023][ T122] GPR04: c000000000515c10 c000000000e5fe08 >> c00000000133da60 c000000004839300 >> [ 67.284023][ T122] GPR08: c0000000014ffb98 0000000000000000 >> c000000000515c0c c000000000e18576 >> [ 67.284023][ T122] GPR12: c000000000e60170 c0000000015a0000 >> 00000001155e0460 0000000000000000 >> [ 67.284023][ T122] GPR16: 0000000000000000 00007fffe8eeb3c8 >> 0000000116320728 0000000000000000 >> [ 67.284023][ T122] GPR20: 0000000116320720 0000000000000000 >> c0000000012fa918 0000000000000006 >> [ 67.284023][ T122] GPR24: c0000000014ffb98 c0000000011ed360 >> 0000000000000000 c000000001fb7928 >> [ 67.284023][ T122] GPR28: 0000000000000000 0000000000000000 >> 000000007c0802a6 c000000001fb7918 >> [ 67.287799][ T122] NIP [c000000000050bfc] >> arch_prepare_kprobe+0x10c/0x2d0 >> [ 67.288490][ T122] LR [c000000000050bec] >> arch_prepare_kprobe+0xfc/0x2d0 >> [ 67.289025][ T122] Call Trace: >> [ 67.289268][ T122] [c000000034847850] [c0000000012f77a0] >> 0xc0000000012f77a0 (unreliable) >> [ 67.289999][ T122] [c0000000348478d0] [c000000000231320] >> register_kprobe+0x3c0/0x7a0 >> [ 67.290439][ T122] [c000000034847940] [c0000000002938c0] >> __register_trace_kprobe+0x140/0x1a0 >> [ 67.290898][ T122] [c0000000348479b0] [c0000000002944c4] >> __trace_kprobe_create+0x794/0x1040 >> [ 67.291330][ T122] [c000000034847b60] [c0000000002a1614] >> trace_probe_create+0xc4/0xe0 >> [ 67.291717][ T122] [c000000034847bb0] [c00000000029363c] >> create_or_delete_trace_kprobe+0x2c/0x80 >> [ 67.292158][ T122] [c000000034847bd0] [c000000000264420] >> trace_parse_run_command+0xf0/0x210 >> [ 67.292611][ T122] [c000000034847c70] [c0000000002934a0] >> probes_write+0x20/0x40 >> [ 67.292996][ T122] [c000000034847c90] [c00000000045e98c] >> vfs_write+0xfc/0x450 >> [ 67.293356][ T122] [c000000034847d50] [c00000000045eec4] >> ksys_write+0x84/0x140 >> [ 67.293716][ T122] [c000000034847da0] [c00000000002e4fc] >> system_call_exception+0x17c/0x3a0 >> [ 67.294186][ T122] [c000000034847e10] [c00000000000c0e8] >> system_call_vectored_common+0xe8/0x278 >> [ 67.294680][ T122] --- interrupt: 3000 at 0x7fffa5682de0 >> [ 67.294937][ T122] NIP: 00007fffa5682de0 LR: 0000000000000000 >> CTR:0000000000000000 >> [ 67.295313][ T122] REGS: c000000034847e80 TRAP: 3000 Not >> tainted (6.0.0-rc3-00007-gdcf8e5633e2e) >> [ 67.295725][ T122] MSR: 900000000280f033 >> <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 44002408 XER: 00000000 >> [ 67.296291][ T122] IRQMASK: 0 >> [ 67.296291][ T122] GPR00: 0000000000000004 00007fffe8eeaec0 >> 00007fffa5757300 0000000000000001 >> [ 67.296291][ T122] GPR04: 0000000116329c60 0000000000000017 >> 0000000000116329 0000000000000000 >> [ 67.296291][ T122] GPR08: 0000000000000006 0000000000000000 >> 0000000000000000 0000000000000000 >> [ 67.296291][ T122] GPR12: 0000000000000000 00007fffa580ac60 >> 00000001155e0460 0000000000000000 >> [ 67.296291][ T122] GPR16: 0000000000000000 00007fffe8eeb3c8 >> 0000000116320728 0000000000000000 >> [ 67.296291][ T122] GPR20: 0000000116320720 0000000000000000 >> 0000000000000000 0000000000000002 >> [ 67.296291][ T122] GPR24: 00000001163206f0 0000000000000020 >> 00007fffe8eeafa0 0000000000000001 >> [ 67.296291][ T122] GPR28: 0000000000000000 0000000000000017 >> 0000000116329c60 0000000000000001 >> [ 67.299570][ T122] NIP [00007fffa5682de0] 0x7fffa5682de0 >> [ 67.299837][ T122] LR [0000000000000000] 0x0 >> [ 67.300072][ T122] --- interrupt: 3000 >> [ 67.300447][ T122] Instruction dump: >> [ 67.300736][ T122] 386319d8 481342f5 60000000 60000000 60000000 >> e87f0028 3863fffc 481dc4d1 >> [ 67.301230][ T122] 60000000 2c230000 41820018 e9230058 >> <81290000> 552936be 2c090001 4182018c >> [ 67.302102][ T122] ---[ end trace 0000000000000000 ]--- >> [ 67.302496][ T122] > > Please consider trimming the backtrace to only capture the necessary > information: > https://docs.kernel.org/process/submitting-patches.html#backtraces-in-commit-mesages > > Thank you for the reminder. Michael has modified it when merging in, thanks Michael. >> >> The address being probed has some special: >> >> cmdline_proc_show: Probe based on ftrace >> cmdline_proc_show+16: Probe for the next instruction at the ftrace >> location >> >> The ftrace-based kprobe does not generate kprobe::ainsn::insn, it gets >> set to NULL. In arch_prepare_kprobe() it will check for: >> >> ... >> prev = get_kprobe(p->addr - 1); >> preempt_enable_no_resched(); >> if (prev && ppc_inst_prefixed(ppc_inst_read(prev->ainsn.insn))) { >> ... >> >> If prev is based on ftrace, 'ppc_inst_read(prev->ainsn.insn)' will occur >> with a null pointer reference. At this point prev->addr will not be a >> prefixed instruction, so the check can be skipped. >> >> Check if prev is ftrace-based kprobe before reading 'prev->ainsn.insn' >> to fix this problem. >> >> Fixes: b4657f7650ba ("powerpc/kprobes: Don't allow breakpoints on >> suffixes") >> Signed-off-by: Li Huafei <lihuafei1@huawei.com> >> --- >> arch/powerpc/kernel/kprobes.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kernel/kprobes.c >> b/arch/powerpc/kernel/kprobes.c >> index 912d4f8a13be..9f6cbbd56809 100644 >> --- a/arch/powerpc/kernel/kprobes.c >> +++ b/arch/powerpc/kernel/kprobes.c >> @@ -161,7 +161,12 @@ int arch_prepare_kprobe(struct kprobe *p) >> preempt_disable(); >> prev = get_kprobe(p->addr - 1); >> preempt_enable_no_resched(); >> - if (prev && ppc_inst_prefixed(ppc_inst_read(prev->ainsn.insn))) { >> + /* >> + * When prev is a ftrace-based kprobe, we don't have an insn, and it >> + * doesn't probe for prefixed instruction. >> + */ >> + if (prev && !kprobe_ftrace(prev) && >> + ppc_inst_prefixed(ppc_inst_read(prev->ainsn.insn))) { >> printk("Cannot register a kprobe on the second word of >> prefixed instruction\n"); >> ret = -EINVAL; >> } > > It's fine to keep the if condition on a single line. > > Other than that, thanks for the fix! > Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > > - Naveen > > .
Li Huafei wrote: >>> >>> # echo 'p cmdline_proc_show' > kprobe_events >>> # echo 'p cmdline_proc_show+16' >> kprobe_events >> >> I think we should extend multiple_kprobes selftest to also place >> contiguous probes to catch such errors. >> > Yes. But each architecture implementation is different and it looks a > little difficult to decide which offsets need to be tested. I don't think we need to be accurate here. A test to simply try putting a probe at every byte within the first 256 bytes of a kernel function should help catch many such issues. Some of those probes will be rejected, but we can ignore errors. - Naveen
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index 912d4f8a13be..9f6cbbd56809 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -161,7 +161,12 @@ int arch_prepare_kprobe(struct kprobe *p) preempt_disable(); prev = get_kprobe(p->addr - 1); preempt_enable_no_resched(); - if (prev && ppc_inst_prefixed(ppc_inst_read(prev->ainsn.insn))) { + /* + * When prev is a ftrace-based kprobe, we don't have an insn, and it + * doesn't probe for prefixed instruction. + */ + if (prev && !kprobe_ftrace(prev) && + ppc_inst_prefixed(ppc_inst_read(prev->ainsn.insn))) { printk("Cannot register a kprobe on the second word of prefixed instruction\n"); ret = -EINVAL; }
I found a null pointer reference in arch_prepare_kprobe(): # echo 'p cmdline_proc_show' > kprobe_events # echo 'p cmdline_proc_show+16' >> kprobe_events [ 67.278533][ T122] Kernel attempted to read user page (0) - exploit attempt? (uid: 0) [ 67.279326][ T122] BUG: Kernel NULL pointer dereference on read at 0x00000000 [ 67.279738][ T122] Faulting instruction address: 0xc000000000050bfc [ 67.280486][ T122] Oops: Kernel access of bad area, sig: 11 [#1] [ 67.280846][ T122] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV [ 67.281435][ T122] Modules linked in: [ 67.281903][ T122] CPU: 0 PID: 122 Comm: sh Not tainted 6.0.0-rc3-00007-gdcf8e5633e2e #10 [ 67.282547][ T122] NIP: c000000000050bfc LR: c000000000050bec CTR: 0000000000005bdc [ 67.282920][ T122] REGS: c0000000348475b0 TRAP: 0300 Not tainted (6.0.0-rc3-00007-gdcf8e5633e2e) [ 67.283424][ T122] MSR: 9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 88002444 XER: 20040006 [ 67.284023][ T122] CFAR: c00000000022d100 DAR: 0000000000000000 DSISR: 40000000 IRQMASK: 0 [ 67.284023][ T122] GPR00: c000000000050bec c000000034847850 c0000000013f6100 c000000001fb7718 [ 67.284023][ T122] GPR04: c000000000515c10 c000000000e5fe08 c00000000133da60 c000000004839300 [ 67.284023][ T122] GPR08: c0000000014ffb98 0000000000000000 c000000000515c0c c000000000e18576 [ 67.284023][ T122] GPR12: c000000000e60170 c0000000015a0000 00000001155e0460 0000000000000000 [ 67.284023][ T122] GPR16: 0000000000000000 00007fffe8eeb3c8 0000000116320728 0000000000000000 [ 67.284023][ T122] GPR20: 0000000116320720 0000000000000000 c0000000012fa918 0000000000000006 [ 67.284023][ T122] GPR24: c0000000014ffb98 c0000000011ed360 0000000000000000 c000000001fb7928 [ 67.284023][ T122] GPR28: 0000000000000000 0000000000000000 000000007c0802a6 c000000001fb7918 [ 67.287799][ T122] NIP [c000000000050bfc] arch_prepare_kprobe+0x10c/0x2d0 [ 67.288490][ T122] LR [c000000000050bec] arch_prepare_kprobe+0xfc/0x2d0 [ 67.289025][ T122] Call Trace: [ 67.289268][ T122] [c000000034847850] [c0000000012f77a0] 0xc0000000012f77a0 (unreliable) [ 67.289999][ T122] [c0000000348478d0] [c000000000231320] register_kprobe+0x3c0/0x7a0 [ 67.290439][ T122] [c000000034847940] [c0000000002938c0] __register_trace_kprobe+0x140/0x1a0 [ 67.290898][ T122] [c0000000348479b0] [c0000000002944c4] __trace_kprobe_create+0x794/0x1040 [ 67.291330][ T122] [c000000034847b60] [c0000000002a1614] trace_probe_create+0xc4/0xe0 [ 67.291717][ T122] [c000000034847bb0] [c00000000029363c] create_or_delete_trace_kprobe+0x2c/0x80 [ 67.292158][ T122] [c000000034847bd0] [c000000000264420] trace_parse_run_command+0xf0/0x210 [ 67.292611][ T122] [c000000034847c70] [c0000000002934a0] probes_write+0x20/0x40 [ 67.292996][ T122] [c000000034847c90] [c00000000045e98c] vfs_write+0xfc/0x450 [ 67.293356][ T122] [c000000034847d50] [c00000000045eec4] ksys_write+0x84/0x140 [ 67.293716][ T122] [c000000034847da0] [c00000000002e4fc] system_call_exception+0x17c/0x3a0 [ 67.294186][ T122] [c000000034847e10] [c00000000000c0e8] system_call_vectored_common+0xe8/0x278 [ 67.294680][ T122] --- interrupt: 3000 at 0x7fffa5682de0 [ 67.294937][ T122] NIP: 00007fffa5682de0 LR: 0000000000000000 CTR: 0000000000000000 [ 67.295313][ T122] REGS: c000000034847e80 TRAP: 3000 Not tainted (6.0.0-rc3-00007-gdcf8e5633e2e) [ 67.295725][ T122] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 44002408 XER: 00000000 [ 67.296291][ T122] IRQMASK: 0 [ 67.296291][ T122] GPR00: 0000000000000004 00007fffe8eeaec0 00007fffa5757300 0000000000000001 [ 67.296291][ T122] GPR04: 0000000116329c60 0000000000000017 0000000000116329 0000000000000000 [ 67.296291][ T122] GPR08: 0000000000000006 0000000000000000 0000000000000000 0000000000000000 [ 67.296291][ T122] GPR12: 0000000000000000 00007fffa580ac60 00000001155e0460 0000000000000000 [ 67.296291][ T122] GPR16: 0000000000000000 00007fffe8eeb3c8 0000000116320728 0000000000000000 [ 67.296291][ T122] GPR20: 0000000116320720 0000000000000000 0000000000000000 0000000000000002 [ 67.296291][ T122] GPR24: 00000001163206f0 0000000000000020 00007fffe8eeafa0 0000000000000001 [ 67.296291][ T122] GPR28: 0000000000000000 0000000000000017 0000000116329c60 0000000000000001 [ 67.299570][ T122] NIP [00007fffa5682de0] 0x7fffa5682de0 [ 67.299837][ T122] LR [0000000000000000] 0x0 [ 67.300072][ T122] --- interrupt: 3000 [ 67.300447][ T122] Instruction dump: [ 67.300736][ T122] 386319d8 481342f5 60000000 60000000 60000000 e87f0028 3863fffc 481dc4d1 [ 67.301230][ T122] 60000000 2c230000 41820018 e9230058 <81290000> 552936be 2c090001 4182018c [ 67.302102][ T122] ---[ end trace 0000000000000000 ]--- [ 67.302496][ T122] The address being probed has some special: cmdline_proc_show: Probe based on ftrace cmdline_proc_show+16: Probe for the next instruction at the ftrace location The ftrace-based kprobe does not generate kprobe::ainsn::insn, it gets set to NULL. In arch_prepare_kprobe() it will check for: ... prev = get_kprobe(p->addr - 1); preempt_enable_no_resched(); if (prev && ppc_inst_prefixed(ppc_inst_read(prev->ainsn.insn))) { ... If prev is based on ftrace, 'ppc_inst_read(prev->ainsn.insn)' will occur with a null pointer reference. At this point prev->addr will not be a prefixed instruction, so the check can be skipped. Check if prev is ftrace-based kprobe before reading 'prev->ainsn.insn' to fix this problem. Fixes: b4657f7650ba ("powerpc/kprobes: Don't allow breakpoints on suffixes") Signed-off-by: Li Huafei <lihuafei1@huawei.com> --- arch/powerpc/kernel/kprobes.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)