| Message ID | 20191224055545.178462-3-ruscur@russell.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement STRICT_MODULE_RWX for powerpc | expand |
Le 24/12/2019 à 06:55, Russell Currey a écrit : > 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> > --- > arch/powerpc/kernel/kprobes.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c > index 2d27ec4feee4..b72761f0c9e3 100644 > --- a/arch/powerpc/kernel/kprobes.c > +++ b/arch/powerpc/kernel/kprobes.c > @@ -24,6 +24,7 @@ > #include <asm/sstep.h> > #include <asm/sections.h> > #include <linux/uaccess.h> > +#include <linux/set_memory.h> > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > @@ -124,13 +125,14 @@ 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)); patch_instruction() already does the flush, no need to flush again with flush_icache_range() > } > > + set_memory_ro((unsigned long)p->ainsn.insn, 1); > + I don't really understand, why do you need to set this ro ? Or why do you need to change the memcpy() to patch_instruction() if the area is not already ro ? If I understand correctly, p->ainsn.insn is within a special executable page allocated via module_alloc(). Wouldn't it be more correct to modify kprobe get_insn_slot() logic so that allocated page is ROX instead of RWX ? > p->ainsn.boostable = 0; > return ret; > } > Christophe
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index 2d27ec4feee4..b72761f0c9e3 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -24,6 +24,7 @@ #include <asm/sstep.h> #include <asm/sections.h> #include <linux/uaccess.h> +#include <linux/set_memory.h> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL; DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); @@ -124,13 +125,14 @@ 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)); } + set_memory_ro((unsigned long)p->ainsn.insn, 1); + p->ainsn.boostable = 0; return ret; }