Re: [PATCH v5 2/5] powerpc/kprobes: Mark newly allocated probes as RO
On Fri, 2019-12-06 at 10:47 +1100, Michael Ellerman wrote: > Michael Ellerman writes: > > Russell Currey writes: > > > With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will > > > be one > > > W+X page at boot by default. This can be tested with > > > CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking > > > the > > > kernel log during boot. > > > > > > powerpc doesn't implement its own alloc() for kprobes like other > > > architectures do, but we couldn't immediately mark RO anyway > > > since we do > > > a memcpy to the page we allocate later. After that, nothing > > > should be > > > allowed to modify the page, and write permissions are removed > > > well > > > before the kprobe is armed. > > > > > > Thus mark newly allocated probes as read-only once it's safe to > > > do so. > > > > > > Signed-off-by: Russell Currey > > > --- > > > arch/powerpc/kernel/kprobes.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/arch/powerpc/kernel/kprobes.c > > > b/arch/powerpc/kernel/kprobes.c > > > index 2d27ec4feee4..2610496de7c7 100644 > > > --- a/arch/powerpc/kernel/kprobes.c > > > +++ b/arch/powerpc/kernel/kprobes.c > > > @@ -24,6 +24,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; > > > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > > > @@ -131,6 +132,8 @@ int arch_prepare_kprobe(struct kprobe *p) > > > (unsigned long)p->ainsn.insn + > > > sizeof(kprobe_opcode_t)); > > > } > > > > > > + set_memory_ro((unsigned long)p->ainsn.insn, 1); > > > + > > > > That comes from: > > p->ainsn.insn = get_insn_slot(); > > > > > > Which ends up in __get_insn_slot() I think. And that looks very > > much > > like it's going to hand out multiple slots per page, which isn't > > going > > to work because you've just marked the whole page RO. > > > > So I would expect this to crash on the 2nd kprobe that's installed. > > Have > > you tested it somehow? > > I'm not sure if this is the issue I was talking about, but it doesn't > survive ftracetest: > > [ 1139.576047] [ cut here ] > [ 1139.576322] kernel BUG at mm/memory.c:2036! > cpu 0x1f: Vector: 700 (Program Check) at [c01fd6c675d0] > pc: c035d018: apply_to_page_range+0x318/0x610 > lr: c00900bc: change_memory_attr+0x4c/0x70 > sp: c01fd6c67860 > msr: 90029033 > current = 0xc01fa4a47880 > paca= 0xc01e5c80 irqmask: 0x03 irq_happened: 0x01 > pid = 7168, comm = ftracetest > kernel BUG at mm/memory.c:2036! > Linux version 5.4.0-gcc-8.2.0-11694-gf1f9aa266811 ( > mich...@raptor-2.ozlabs.ibm.com) (gcc version 8.2.0 (crosstool-NG > 1.24.0-rc1.16-9627a04)) #1384 SMP Thu Dec 5 22:11:09 AEDT 2019 > enter ? for help > [c01fd6c67940] c00900bc change_memory_attr+0x4c/0x70 > [c01fd6c67970] c0053c48 arch_prepare_kprobe+0xb8/0x120 > [c01fd6c679e0] c022f718 register_kprobe+0x608/0x790 > [c01fd6c67a40] c022fc50 register_kretprobe+0x230/0x350 > [c01fd6c67a80] c02849b4 > __register_trace_kprobe+0xf4/0x1a0 > [c01fd6c67af0] c0285b18 trace_kprobe_create+0x738/0xf70 > [c01fd6c67c30] c0286378 > create_or_delete_trace_kprobe+0x28/0x70 > [c01fd6c67c50] c025f024 trace_run_command+0xc4/0xe0 > [c01fd6c67ca0] c025f128 > trace_parse_run_command+0xe8/0x230 > [c01fd6c67d40] c02845d0 probes_write+0x20/0x40 > [c01fd6c67d60] c03eef4c __vfs_write+0x3c/0x70 > [c01fd6c67d80] c03f26a0 vfs_write+0xd0/0x200 > [c01fd6c67dd0] c03f2a3c ksys_write+0x7c/0x140 > [c01fd6c67e20] c000b9e0 system_call+0x5c/0x68 > --- Exception: c01 (System Call) at 7fff8f06e420 > SP (793d6830) is in userspace > 1f:mon> client_loop: send disconnect: Broken pipe > > > Sorry I didn't get any more info on the crash, I lost the console and > then some CI bot stole the machine 8) > > You should be able to reproduce just by running ftracetest. The test that blew it up was test.d/kprobe/probepoint.tc for the record. It goes away when replacing the memcpy with a patch_instruction(). > > cheers
Re: [PATCH v5 2/5] powerpc/kprobes: Mark newly allocated probes as RO
Michael Ellerman writes: > Russell Currey writes: >> With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one >> W+X page at boot by default. This can be tested with >> CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the >> kernel log during boot. >> >> powerpc doesn't implement its own alloc() for kprobes like other >> architectures do, but we couldn't immediately mark RO anyway since we do >> a memcpy to the page we allocate later. After that, nothing should be >> allowed to modify the page, and write permissions are removed well >> before the kprobe is armed. >> >> Thus mark newly allocated probes as read-only once it's safe to do so. >> >> Signed-off-by: Russell Currey >> --- >> arch/powerpc/kernel/kprobes.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c >> index 2d27ec4feee4..2610496de7c7 100644 >> --- a/arch/powerpc/kernel/kprobes.c >> +++ b/arch/powerpc/kernel/kprobes.c >> @@ -24,6 +24,7 @@ >> #include >> #include >> #include >> +#include >> >> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; >> DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); >> @@ -131,6 +132,8 @@ int arch_prepare_kprobe(struct kprobe *p) >> (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t)); >> } >> >> +set_memory_ro((unsigned long)p->ainsn.insn, 1); >> + > > That comes from: > p->ainsn.insn = get_insn_slot(); > > > Which ends up in __get_insn_slot() I think. And that looks very much > like it's going to hand out multiple slots per page, which isn't going > to work because you've just marked the whole page RO. > > So I would expect this to crash on the 2nd kprobe that's installed. Have > you tested it somehow? I'm not sure if this is the issue I was talking about, but it doesn't survive ftracetest: [ 1139.576047] [ cut here ] [ 1139.576322] kernel BUG at mm/memory.c:2036! cpu 0x1f: Vector: 700 (Program Check) at [c01fd6c675d0] pc: c035d018: apply_to_page_range+0x318/0x610 lr: c00900bc: change_memory_attr+0x4c/0x70 sp: c01fd6c67860 msr: 90029033 current = 0xc01fa4a47880 paca= 0xc01e5c80 irqmask: 0x03 irq_happened: 0x01 pid = 7168, comm = ftracetest kernel BUG at mm/memory.c:2036! Linux version 5.4.0-gcc-8.2.0-11694-gf1f9aa266811 (mich...@raptor-2.ozlabs.ibm.com) (gcc version 8.2.0 (crosstool-NG 1.24.0-rc1.16-9627a04)) #1384 SMP Thu Dec 5 22:11:09 AEDT 2019 enter ? for help [c01fd6c67940] c00900bc change_memory_attr+0x4c/0x70 [c01fd6c67970] c0053c48 arch_prepare_kprobe+0xb8/0x120 [c01fd6c679e0] c022f718 register_kprobe+0x608/0x790 [c01fd6c67a40] c022fc50 register_kretprobe+0x230/0x350 [c01fd6c67a80] c02849b4 __register_trace_kprobe+0xf4/0x1a0 [c01fd6c67af0] c0285b18 trace_kprobe_create+0x738/0xf70 [c01fd6c67c30] c0286378 create_or_delete_trace_kprobe+0x28/0x70 [c01fd6c67c50] c025f024 trace_run_command+0xc4/0xe0 [c01fd6c67ca0] c025f128 trace_parse_run_command+0xe8/0x230 [c01fd6c67d40] c02845d0 probes_write+0x20/0x40 [c01fd6c67d60] c03eef4c __vfs_write+0x3c/0x70 [c01fd6c67d80] c03f26a0 vfs_write+0xd0/0x200 [c01fd6c67dd0] c03f2a3c ksys_write+0x7c/0x140 [c01fd6c67e20] c000b9e0 system_call+0x5c/0x68 --- Exception: c01 (System Call) at 7fff8f06e420 SP (793d6830) is in userspace 1f:mon> client_loop: send disconnect: Broken pipe Sorry I didn't get any more info on the crash, I lost the console and then some CI bot stole the machine 8) You should be able to reproduce just by running ftracetest. cheers
Re: [PATCH v5 2/5] powerpc/kprobes: Mark newly allocated probes as RO
Russell Currey writes: > With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one > W+X page at boot by default. This can be tested with > CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the > kernel log during boot. > > powerpc doesn't implement its own alloc() for kprobes like other > architectures do, but we couldn't immediately mark RO anyway since we do > a memcpy to the page we allocate later. After that, nothing should be > allowed to modify the page, and write permissions are removed well > before the kprobe is armed. > > Thus mark newly allocated probes as read-only once it's safe to do so. > > Signed-off-by: Russell Currey > --- > arch/powerpc/kernel/kprobes.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c > index 2d27ec4feee4..2610496de7c7 100644 > --- a/arch/powerpc/kernel/kprobes.c > +++ b/arch/powerpc/kernel/kprobes.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > @@ -131,6 +132,8 @@ int arch_prepare_kprobe(struct kprobe *p) > (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t)); > } > > + set_memory_ro((unsigned long)p->ainsn.insn, 1); > + That comes from: p->ainsn.insn = get_insn_slot(); Which ends up in __get_insn_slot() I think. And that looks very much like it's going to hand out multiple slots per page, which isn't going to work because you've just marked the whole page RO. So I would expect this to crash on the 2nd kprobe that's installed. Have you tested it somehow? I think this code should just use patch_instruction() rather than memcpy(). cheers
Re: [PATCH v5 2/5] powerpc/kprobes: Mark newly allocated probes as RO
Russell Currey writes: > With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one > W+X page at boot by default. This can be tested with > CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the > kernel log during boot. > > powerpc doesn't implement its own alloc() for kprobes like other > architectures do, but we couldn't immediately mark RO anyway since we do > a memcpy to the page we allocate later. After that, nothing should be > allowed to modify the page, and write permissions are removed well > before the kprobe is armed. > > Thus mark newly allocated probes as read-only once it's safe to do so. So if I've got the flow right here: register[_aggr]_kprobe -> prepare_kprobe -> arch_prepare_kprobe perform memcpy mark as readonly, after which no-one touches writes to the memory which all seems right to me. I have been trying to check if optprobes need special handling: it looks like the buffer for them lives in kernel text, not dynamically allocated memory, so it should be protected by the usual Strict RWX protections without special treatment here. So lgtm. Reviewed-by: Daniel Axtens Regards, Daniel > > Signed-off-by: Russell Currey > --- > arch/powerpc/kernel/kprobes.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c > index 2d27ec4feee4..2610496de7c7 100644 > --- a/arch/powerpc/kernel/kprobes.c > +++ b/arch/powerpc/kernel/kprobes.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > @@ -131,6 +132,8 @@ int arch_prepare_kprobe(struct kprobe *p) > (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t)); > } > > + set_memory_ro((unsigned long)p->ainsn.insn, 1); > + > p->ainsn.boostable = 0; > return ret; > } > -- > 2.23.0
[PATCH v5 2/5] powerpc/kprobes: Mark newly allocated probes as RO
With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one W+X page at boot by default. This can be tested with CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the kernel log during boot. powerpc doesn't implement its own alloc() for kprobes like other architectures do, but we couldn't immediately mark RO anyway since we do a memcpy to the page we allocate later. After that, nothing should be allowed to modify the page, and write permissions are removed well before the kprobe is armed. Thus mark newly allocated probes as read-only once it's safe to do so. Signed-off-by: Russell Currey --- arch/powerpc/kernel/kprobes.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index 2d27ec4feee4..2610496de7c7 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -24,6 +24,7 @@ #include #include #include +#include DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); @@ -131,6 +132,8 @@ int arch_prepare_kprobe(struct kprobe *p) (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t)); } + set_memory_ro((unsigned long)p->ainsn.insn, 1); + p->ainsn.boostable = 0; return ret; } -- 2.23.0
