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 <d...@axtens.net> > > > Signed-off-by: Russell Currey <rus...@russell.cc> > > > Signed-off-by: Christophe Leroy <christophe.le...@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 >