[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <51b9b43b-9f25-bb68-93f2-cd5ba7d67f38@c-s.fr>
Date: Wed, 8 Jan 2020 17:48:59 +0100
From: Christophe Leroy <christophe.leroy@....fr>
To: Russell Currey <ruscur@...sell.cc>, linuxppc-dev@...ts.ozlabs.org
Cc: joel@....id.au, mpe@...erman.id.au, ajd@...ux.ibm.com, dja@...ens.net,
npiggin@...il.com, kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v6 2/5] powerpc/kprobes: Mark newly allocated probes as RO
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@...ens.net>
> Signed-off-by: Russell Currey <ruscur@...sell.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
