* Masami Hiramatsu <mhira...@kernel.org> wrote:
> Do not forget to set kprobes insn buffer memory back
> to RO on failure path. Without this fix, if there is
> an unexpected error on copying instructions, kprobes
> insn buffer kept RW, which can allow unexpected modifying
> instruction buffer.
>
> Fixes: d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
> Signed-off-by: Masami Hiramatsu <mhira...@kernel.org>
> ---
> arch/x86/kernel/kprobes/core.c | 4 +++-
> arch/x86/kernel/kprobes/opt.c | 1 +
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index f0153714ddac..b16b10114e20 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -435,8 +435,10 @@ static int arch_copy_kprobe(struct kprobe *p)
>
> /* Copy an instruction with recovering if other optprobe modifies it.*/
> len = __copy_instruction(p->ainsn.insn, p->addr, &insn);
> - if (!len)
> + if (!len) {
> + set_memory_ro((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
> return -EINVAL;
> + }
So variable usage in the arch_copy_kprobe() is really awful: 'p->ainsn.insn' is
repeated 6 times!
Please consolidate all that via a helper variable.
Also, regarding the merits of the patch: do we know that the page in question
was
RO before? If it was RW we'll unexpectedly mark it RO here in the failure path
...
> index 69ea0bc1cfa3..853614560a4f 100644
> --- a/arch/x86/kernel/kprobes/opt.c
> +++ b/arch/x86/kernel/kprobes/opt.c
> @@ -368,6 +368,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe
> *op,
> ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr);
> if (ret < 0) {
> __arch_remove_optimized_kprobe(op, 0);
> + set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
> return ret;
> }
> op->optinsn.size = ret;
Ditto.
Thanks,
Ingo
