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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 12 Aug 2017 08:09:52 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Ingo Molnar <mingo@...hat.com>, "H . Peter Anvin" <hpa@...or.com>,
        x86@...nel.org, Ananth N Mavinakayanahalli <ananth@...ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH -tip 1/2] kprobes/x86: Don't forget to set memory back
 to RO on failure

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

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

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@...nel.org>