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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20170810152956.n3dhwjtv2nyxmrpr@gmail.com>
Date:   Thu, 10 Aug 2017 17:29:56 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Masami Hiramatsu <mhiramat@...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


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

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