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

[Masami Hiramatsu]

On Thu, 10 Aug 2017 17:29:56 +0200
Ingo Molnar <mingo@xxxxxxxxxx> wrote:

> 
> * 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.

OK, I'll cleanup it soon.

> 
> 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 ...

No need to take care previous state, this page has to be RO after setup (even if
it was failed), since the page is shared by other kprobes. If we missed it,
insn buffers for other kprobes will be left in RW state.

> > 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.

As same as above, this page is shared by other optprobes.

Thank you,

> 
> Thanks,
> 
> 	Ingo


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>