| Message ID | 20200402084053.188537-2-ruscur@russell.cc (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v8,1/7] powerpc/mm: Implement set_memory() routines | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (d0c12846a3a24cd6d68b608c866712bc7e471634) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 36 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Russell Currey wrote: > 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. > > The memcpy() would fail if >1 probes were allocated, so use > patch_instruction() instead which is safe for RO. > > Reviewed-by: Daniel Axtens <dja@axtens.net> > Signed-off-by: Russell Currey <ruscur@russell.cc> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > arch/powerpc/kernel/kprobes.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c > index 81efb605113e..fa4502b4de35 100644 > --- a/arch/powerpc/kernel/kprobes.c > +++ b/arch/powerpc/kernel/kprobes.c > @@ -24,6 +24,8 @@ > #include <asm/sstep.h> > #include <asm/sections.h> > #include <linux/uaccess.h> > +#include <linux/set_memory.h> > +#include <linux/vmalloc.h> > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > @@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) > return addr; > } > > +void *alloc_insn_page(void) > +{ > + void *page = vmalloc_exec(PAGE_SIZE); > + > + if (page) > + set_memory_ro((unsigned long)page, 1); > + > + return page; > +} > + This crashes for me with KPROBES_SANITY_TEST during the kretprobe test. It seems to be handling the kretprobe itself properly, but seems to crash on the return path. I haven't yet been able to work out what's going wrong. [ 0.517880] Kprobe smoke test: started [ 0.626680] Oops: Exception in kernel mode, sig: 4 [#1] [ 0.626708] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV [ 0.626735] Modules linked in: [ 0.626760] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.6.0-06592-g2e64694b9137 #51 [ 0.626795] NIP: c008000000020000 LR: c00000000021ce34 CTR: c00000000021c5f8 [ 0.626829] REGS: c0000000fd3e3860 TRAP: 0e40 Not tainted (5.6.0-06592-g2e64694b9137) [ 0.626862] MSR: 9000000002089433 <SF,HV,VEC,EE,ME,SE,IR,DR,RI,LE> CR: 28000284 XER: 00040000 [ 0.626922] CFAR: c00000000000ef20 IRQMASK: 0 [ 0.626922] GPR00: c000000000052250 c0000000fd3e3af0 c000000002330200 000000002db8ad86 [ 0.626922] GPR04: 0000000000000000 c00000000006ba3c 0000000000000800 c0000000fd3a0000 [ 0.626922] GPR08: 0000000000000000 ffffffffaaaaaaab c0000000fd3a0000 0000000040000000 [ 0.626922] GPR12: c00000000021c5f0 c000000002520000 c000000000011790 0000000000000000 [ 0.626922] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 0.626922] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 0.626922] GPR24: c0000000020034bc c0000000012068b8 c000000002062e50 c0000000fd2319a0 [ 0.626922] GPR28: c000000000f5ebb0 0000000000000000 c0000000021bc278 c000000002458540 [ 0.627234] NIP [c008000000020000] 0xc008000000020000 [ 0.627264] LR [c00000000021ce34] init_test_probes+0x424/0x560 [ 0.627291] Call Trace: [ 0.627313] [c0000000fd3e3af0] [c00000000021ce34] init_test_probes+0x424/0x560 (unreliable) [ 0.627356] [c0000000fd3e3b90] [c00000000202de2c] init_kprobes+0x1a8/0x1c8 [ 0.627392] [c0000000fd3e3c00] [c000000000011140] do_one_initcall+0x60/0x2b0 [ 0.627432] [c0000000fd3e3cd0] [c000000002004674] kernel_init_freeable+0x2e0/0x3a0 [ 0.627471] [c0000000fd3e3db0] [c0000000000117ac] kernel_init+0x24/0x178 [ 0.627510] [c0000000fd3e3e20] [c00000000000c7a8] ret_from_kernel_thread+0x5c/0x74 [ 0.627543] Instruction dump: [ 0.627562] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX [ 0.627607] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX <00000000> 00000000 00000000 00000000 [ 0.627660] ---[ end trace 964ab92781f5d99d ]--- [ 0.629607] - Naveen
Naveen N. Rao wrote: > Russell Currey wrote: >> 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. >> >> The memcpy() would fail if >1 probes were allocated, so use >> patch_instruction() instead which is safe for RO. >> >> Reviewed-by: Daniel Axtens <dja@axtens.net> >> Signed-off-by: Russell Currey <ruscur@russell.cc> >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >> --- >> arch/powerpc/kernel/kprobes.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c >> index 81efb605113e..fa4502b4de35 100644 >> --- a/arch/powerpc/kernel/kprobes.c >> +++ b/arch/powerpc/kernel/kprobes.c >> @@ -24,6 +24,8 @@ >> #include <asm/sstep.h> >> #include <asm/sections.h> >> #include <linux/uaccess.h> >> +#include <linux/set_memory.h> >> +#include <linux/vmalloc.h> >> >> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; >> DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); >> @@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) >> return addr; >> } >> >> +void *alloc_insn_page(void) >> +{ >> + void *page = vmalloc_exec(PAGE_SIZE); >> + >> + if (page) >> + set_memory_ro((unsigned long)page, 1); >> + >> + return page; >> +} >> + > > This crashes for me with KPROBES_SANITY_TEST during the kretprobe test. That isn't needed to reproduce this. After bootup, disabling optprobes also shows the crash with kretprobes: sysctl debug.kprobes-optimization=0 The problem happens to be with patch_instruction() in arch_prepare_kprobe(). During boot, on kprobe init, we register a probe on kretprobe_trampoline for use with kretprobes (see arch_init_kprobes()). This results in an instruction slot being allocated, and arch_prepare_kprobe() to be called for copying the instruction (nop) at kretprobe_trampoline. patch_instruction() is failing resulting in corrupt instruction which we try to emulate/single step causing the crash. - Naveen
On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote: > Naveen N. Rao wrote: > > Russell Currey wrote: > > > 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. > > > > > > The memcpy() would fail if >1 probes were allocated, so use > > > patch_instruction() instead which is safe for RO. > > > > > > Reviewed-by: Daniel Axtens <dja@axtens.net> > > > Signed-off-by: Russell Currey <ruscur@russell.cc> > > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > > > --- > > > arch/powerpc/kernel/kprobes.c | 17 +++++++++++++---- > > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/powerpc/kernel/kprobes.c > > > b/arch/powerpc/kernel/kprobes.c > > > index 81efb605113e..fa4502b4de35 100644 > > > --- a/arch/powerpc/kernel/kprobes.c > > > +++ b/arch/powerpc/kernel/kprobes.c > > > @@ -24,6 +24,8 @@ > > > #include <asm/sstep.h> > > > #include <asm/sections.h> > > > #include <linux/uaccess.h> > > > +#include <linux/set_memory.h> > > > +#include <linux/vmalloc.h> > > > > > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; > > > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > > > @@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const > > > char *name, unsigned int offset) > > > return addr; > > > } > > > > > > +void *alloc_insn_page(void) > > > +{ > > > + void *page = vmalloc_exec(PAGE_SIZE); > > > + > > > + if (page) > > > + set_memory_ro((unsigned long)page, 1); > > > + > > > + return page; > > > +} > > > + > > > > This crashes for me with KPROBES_SANITY_TEST during the kretprobe > > test. > > That isn't needed to reproduce this. After bootup, disabling > optprobes > also shows the crash with kretprobes: > sysctl debug.kprobes-optimization=0 > > The problem happens to be with patch_instruction() in > arch_prepare_kprobe(). During boot, on kprobe init, we register a > probe > on kretprobe_trampoline for use with kretprobes (see > arch_init_kprobes()). This results in an instruction slot being > allocated, and arch_prepare_kprobe() to be called for copying the > instruction (nop) at kretprobe_trampoline. patch_instruction() is > failing resulting in corrupt instruction which we try to > emulate/single > step causing the crash. Thanks a lot for the info Naveen, I'll investigate. - Russell > > > - Naveen >
On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote: > Naveen N. Rao wrote: > > Russell Currey wrote: > > > 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. > > > > > > The memcpy() would fail if >1 probes were allocated, so use > > > patch_instruction() instead which is safe for RO. > > > > > > Reviewed-by: Daniel Axtens <dja@axtens.net> > > > Signed-off-by: Russell Currey <ruscur@russell.cc> > > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > > > --- > > > arch/powerpc/kernel/kprobes.c | 17 +++++++++++++---- > > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/powerpc/kernel/kprobes.c > > > b/arch/powerpc/kernel/kprobes.c > > > index 81efb605113e..fa4502b4de35 100644 > > > --- a/arch/powerpc/kernel/kprobes.c > > > +++ b/arch/powerpc/kernel/kprobes.c > > > @@ -24,6 +24,8 @@ > > > #include <asm/sstep.h> > > > #include <asm/sections.h> > > > #include <linux/uaccess.h> > > > +#include <linux/set_memory.h> > > > +#include <linux/vmalloc.h> > > > > > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; > > > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > > > @@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const > > > char *name, unsigned int offset) > > > return addr; > > > } > > > > > > +void *alloc_insn_page(void) > > > +{ > > > + void *page = vmalloc_exec(PAGE_SIZE); > > > + > > > + if (page) > > > + set_memory_ro((unsigned long)page, 1); > > > + > > > + return page; > > > +} > > > + > > > > This crashes for me with KPROBES_SANITY_TEST during the kretprobe > > test. > > That isn't needed to reproduce this. After bootup, disabling > optprobes > also shows the crash with kretprobes: > sysctl debug.kprobes-optimization=0 > > The problem happens to be with patch_instruction() in > arch_prepare_kprobe(). During boot, on kprobe init, we register a > probe > on kretprobe_trampoline for use with kretprobes (see > arch_init_kprobes()). This results in an instruction slot being > allocated, and arch_prepare_kprobe() to be called for copying the > instruction (nop) at kretprobe_trampoline. patch_instruction() is > failing resulting in corrupt instruction which we try to > emulate/single > step causing the crash. OK I think I've fixed it, KPROBES_SANITY_TEST passes too. I'd appreciate it if you could test v9, and thanks again for finding this - very embarrassing bug on my side. - Russell > > > - Naveen >
Russell Currey wrote: > On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote: >> Naveen N. Rao wrote: >> > Russell Currey wrote: >> > > 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. >> > > >> > > The memcpy() would fail if >1 probes were allocated, so use >> > > patch_instruction() instead which is safe for RO. >> > > >> > > Reviewed-by: Daniel Axtens <dja@axtens.net> >> > > Signed-off-by: Russell Currey <ruscur@russell.cc> >> > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >> > > --- >> > > arch/powerpc/kernel/kprobes.c | 17 +++++++++++++---- >> > > 1 file changed, 13 insertions(+), 4 deletions(-) >> > > >> > > diff --git a/arch/powerpc/kernel/kprobes.c >> > > b/arch/powerpc/kernel/kprobes.c >> > > index 81efb605113e..fa4502b4de35 100644 >> > > --- a/arch/powerpc/kernel/kprobes.c >> > > +++ b/arch/powerpc/kernel/kprobes.c >> > > @@ -24,6 +24,8 @@ >> > > #include <asm/sstep.h> >> > > #include <asm/sections.h> >> > > #include <linux/uaccess.h> >> > > +#include <linux/set_memory.h> >> > > +#include <linux/vmalloc.h> >> > > >> > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; >> > > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); >> > > @@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const >> > > char *name, unsigned int offset) >> > > return addr; >> > > } >> > > >> > > +void *alloc_insn_page(void) >> > > +{ >> > > + void *page = vmalloc_exec(PAGE_SIZE); >> > > + >> > > + if (page) >> > > + set_memory_ro((unsigned long)page, 1); >> > > + >> > > + return page; >> > > +} >> > > + >> > >> > This crashes for me with KPROBES_SANITY_TEST during the kretprobe >> > test. >> >> That isn't needed to reproduce this. After bootup, disabling >> optprobes >> also shows the crash with kretprobes: >> sysctl debug.kprobes-optimization=0 >> >> The problem happens to be with patch_instruction() in >> arch_prepare_kprobe(). During boot, on kprobe init, we register a >> probe >> on kretprobe_trampoline for use with kretprobes (see >> arch_init_kprobes()). This results in an instruction slot being >> allocated, and arch_prepare_kprobe() to be called for copying the >> instruction (nop) at kretprobe_trampoline. patch_instruction() is >> failing resulting in corrupt instruction which we try to >> emulate/single >> step causing the crash. > > OK I think I've fixed it, KPROBES_SANITY_TEST passes too. I'd > appreciate it if you could test v9, and thanks again for finding this - > very embarrassing bug on my side. Great! Thanks. I think I should also add appropriate error checking to kprobes' use of patch_instruction() which would have caught this much more easily. On a related note, I notice that x86 seems to prefer not having any RWX pages, and so they continue to do 'module_alloc()' followed by 'set_memory_ro()' and then 'set_memory_x()'. Is that something worth following for powerpc? - Naveen
On Fri, 2020-04-03 at 15:06 +0530, Naveen N. Rao wrote: > Russell Currey wrote: > > On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote: > > > Naveen N. Rao wrote: > > > > Russell Currey wrote: > > > > > 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. > > > > > > > > > > The memcpy() would fail if >1 probes were allocated, so use > > > > > patch_instruction() instead which is safe for RO. > > > > > > > > > > Reviewed-by: Daniel Axtens <dja@axtens.net> > > > > > Signed-off-by: Russell Currey <ruscur@russell.cc> > > > > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > > > > > --- > > > > > arch/powerpc/kernel/kprobes.c | 17 +++++++++++++---- > > > > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/arch/powerpc/kernel/kprobes.c > > > > > b/arch/powerpc/kernel/kprobes.c > > > > > index 81efb605113e..fa4502b4de35 100644 > > > > > --- a/arch/powerpc/kernel/kprobes.c > > > > > +++ b/arch/powerpc/kernel/kprobes.c > > > > > @@ -24,6 +24,8 @@ > > > > > #include <asm/sstep.h> > > > > > #include <asm/sections.h> > > > > > #include <linux/uaccess.h> > > > > > +#include <linux/set_memory.h> > > > > > +#include <linux/vmalloc.h> > > > > > > > > > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; > > > > > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > > > > > @@ -102,6 +104,16 @@ kprobe_opcode_t > > > > > *kprobe_lookup_name(const > > > > > char *name, unsigned int offset) > > > > > return addr; > > > > > } > > > > > > > > > > +void *alloc_insn_page(void) > > > > > +{ > > > > > + void *page = vmalloc_exec(PAGE_SIZE); > > > > > + > > > > > + if (page) > > > > > + set_memory_ro((unsigned long)page, 1); > > > > > + > > > > > + return page; > > > > > +} > > > > > + > > > > > > > > This crashes for me with KPROBES_SANITY_TEST during the > > > > kretprobe > > > > test. > > > > > > That isn't needed to reproduce this. After bootup, disabling > > > optprobes > > > also shows the crash with kretprobes: > > > sysctl debug.kprobes-optimization=0 > > > > > > The problem happens to be with patch_instruction() in > > > arch_prepare_kprobe(). During boot, on kprobe init, we register a > > > probe > > > on kretprobe_trampoline for use with kretprobes (see > > > arch_init_kprobes()). This results in an instruction slot being > > > allocated, and arch_prepare_kprobe() to be called for copying > > > the > > > instruction (nop) at kretprobe_trampoline. patch_instruction() > > > is > > > failing resulting in corrupt instruction which we try to > > > emulate/single > > > step causing the crash. > > > > OK I think I've fixed it, KPROBES_SANITY_TEST passes too. I'd > > appreciate it if you could test v9, and thanks again for finding > > this - > > very embarrassing bug on my side. > > Great! Thanks. > > I think I should also add appropriate error checking to kprobes' use > of > patch_instruction() which would have caught this much more easily. Only kind of! It turns out that if the initial setup fails for KPROBES_SANITY_TEST, it silently doesn't run - so you miss the "Kprobe smoke test" text, but you don't get any kind of error either. I'll send a patch so that it fails more loudly later. > > On a related note, I notice that x86 seems to prefer not having any > RWX > pages, and so they continue to do 'module_alloc()' followed by > 'set_memory_ro()' and then 'set_memory_x()'. Is that something worth > following for powerpc? I just noticed that too. arm64 doesn't set theirs executable, as far as I can tell powerpc doesn't need to. > > - Naveen >
Russell Currey wrote: > On Fri, 2020-04-03 at 15:06 +0530, Naveen N. Rao wrote: >> Russell Currey wrote: >> > On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote: >> > > Naveen N. Rao wrote: >> > > > Russell Currey wrote: >> > > > > >> > > > > +void *alloc_insn_page(void) >> > > > > +{ >> > > > > + void *page = vmalloc_exec(PAGE_SIZE); >> > > > > + >> > > > > + if (page) >> > > > > + set_memory_ro((unsigned long)page, 1); >> > > > > + >> > > > > + return page; >> > > > > +} >> > > > > + >> > > > >> > > > This crashes for me with KPROBES_SANITY_TEST during the >> > > > kretprobe >> > > > test. >> > > >> > > That isn't needed to reproduce this. After bootup, disabling >> > > optprobes >> > > also shows the crash with kretprobes: >> > > sysctl debug.kprobes-optimization=0 >> > > >> > > The problem happens to be with patch_instruction() in >> > > arch_prepare_kprobe(). During boot, on kprobe init, we register a >> > > probe >> > > on kretprobe_trampoline for use with kretprobes (see >> > > arch_init_kprobes()). This results in an instruction slot being >> > > allocated, and arch_prepare_kprobe() to be called for copying >> > > the >> > > instruction (nop) at kretprobe_trampoline. patch_instruction() >> > > is >> > > failing resulting in corrupt instruction which we try to >> > > emulate/single >> > > step causing the crash. >> > >> > OK I think I've fixed it, KPROBES_SANITY_TEST passes too. I'd >> > appreciate it if you could test v9, and thanks again for finding >> > this - >> > very embarrassing bug on my side. >> >> Great! Thanks. >> >> I think I should also add appropriate error checking to kprobes' use >> of >> patch_instruction() which would have caught this much more easily. > > Only kind of! It turns out that if the initial setup fails for > KPROBES_SANITY_TEST, it silently doesn't run - so you miss the "Kprobe > smoke test" text, but you don't get any kind of error either. I'll > send a patch so that it fails more loudly later. Ha, I see what you mean. Good catch, we should pass the kprobe init status to the test and have it error out. > >> >> On a related note, I notice that x86 seems to prefer not having any >> RWX >> pages, and so they continue to do 'module_alloc()' followed by >> 'set_memory_ro()' and then 'set_memory_x()'. Is that something worth >> following for powerpc? > > I just noticed that too. arm64 doesn't set theirs executable, as far > as I can tell powerpc doesn't need to. I didn't follow that. We do need it to be executable so that we can single step the original instruction. arm64 does vmalloc_exec(), which looks like it sets the page to RWX, then marks it RO. There is a small window where the page would be WX. x86 instead seems to first allocate the page as RW, mark as RO, and only then enable X - removing that small window where the page is both W and X. - Naveen
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index 81efb605113e..fa4502b4de35 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -24,6 +24,8 @@ #include <asm/sstep.h> #include <asm/sections.h> #include <linux/uaccess.h> +#include <linux/set_memory.h> +#include <linux/vmalloc.h> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); @@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) return addr; } +void *alloc_insn_page(void) +{ + void *page = vmalloc_exec(PAGE_SIZE); + + if (page) + set_memory_ro((unsigned long)page, 1); + + return page; +} + int arch_prepare_kprobe(struct kprobe *p) { int ret = 0; @@ -124,11 +136,8 @@ int arch_prepare_kprobe(struct kprobe *p) } if (!ret) { - memcpy(p->ainsn.insn, p->addr, - MAX_INSN_SIZE * sizeof(kprobe_opcode_t)); + patch_instruction(p->ainsn.insn, *p->addr); p->opcode = *p->addr; - flush_icache_range((unsigned long)p->ainsn.insn, - (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t)); } p->ainsn.boostable = 0;