Re: [PATCH -tip 1/2] kprobes/x86: Don't forget to set memory back to RO on failure

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@xxxxxxxxxx> 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 <mhiramat@xxxxxxxxxx>
> ---
>  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