linux-kernel - Re: [PATCH 08/15] x86/alternatives: Teach text_poke

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: <7C13A4B6-6D5B-44C4-B238-58DC5926D7E1@vmware.com>
Date:   Fri, 7 Jun 2019 05:41:42 +0000
From:   Nadav Amit <namit@...are.com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     the arch/x86 maintainers <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Andy Lutomirski <luto@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Jason Baron <jbaron@...mai.com>, Jiri Kosina <jkosina@...e.cz>,
        David Laight <David.Laight@...LAB.COM>,
        Borislav Petkov <bp@...en8.de>,
        Julia Cartwright <julia@...com>, Jessica Yu <jeyu@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Edward Cree <ecree@...arflare.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>
Subject: Re: [PATCH 08/15] x86/alternatives: Teach text_poke_bp() to emulate
 instructions

> On Jun 5, 2019, at 6:08 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> 
> In preparation for static_call support, teach text_poke_bp() to
> emulate instructions, including CALL.
> 
> The current text_poke_bp() takes a @handler argument which is used as
> a jump target when the temporary INT3 is hit by a different CPU.
> 
> When patching CALL instructions, this doesn't work because we'd miss
> the PUSH of the return address. Instead, teach poke_int3_handler() to
> emulate an instruction, typically the instruction we're patching in.
> 
> This fits almost all text_poke_bp() users, except
> arch_unoptimize_kprobe() which restores random text, and for that site
> we have to build an explicit emulate instruction.
> 
> Cc: Daniel Bristot de Oliveira <bristot@...hat.com>
> Cc: Nadav Amit <namit@...are.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
> arch/x86/include/asm/text-patching.h |    2 -
> arch/x86/kernel/alternative.c        |   47 ++++++++++++++++++++++++++---------
> arch/x86/kernel/jump_label.c         |    3 --
> arch/x86/kernel/kprobes/opt.c        |   11 +++++---
> 4 files changed, 46 insertions(+), 17 deletions(-)
> 
> --- a/arch/x86/include/asm/text-patching.h
> +++ b/arch/x86/include/asm/text-patching.h
> @@ -37,7 +37,7 @@ extern void text_poke_early(void *addr,
> extern void *text_poke(void *addr, const void *opcode, size_t len);
> extern void *text_poke_kgdb(void *addr, const void *opcode, size_t len);
> extern int poke_int3_handler(struct pt_regs *regs);
> -extern void text_poke_bp(void *addr, const void *opcode, size_t len, void *handler);
> +extern void text_poke_bp(void *addr, const void *opcode, size_t len, const void *emulate);
> extern int after_bootmem;
> extern __ro_after_init struct mm_struct *poking_mm;
> extern __ro_after_init unsigned long poking_addr;
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -921,19 +921,25 @@ static void do_sync_core(void *info)
> }
> 
> static bool bp_patching_in_progress;
> -static void *bp_int3_handler, *bp_int3_addr;
> +static const void *bp_int3_opcode, *bp_int3_addr;
> 
> int poke_int3_handler(struct pt_regs *regs)
> {
> +	long ip = regs->ip - INT3_INSN_SIZE + CALL_INSN_SIZE;
> +	struct opcode {
> +		u8 insn;
> +		s32 rel;
> +	} __packed opcode;
> +
> 	/*
> 	 * Having observed our INT3 instruction, we now must observe
> 	 * bp_patching_in_progress.
> 	 *
> -	 * 	in_progress = TRUE		INT3
> -	 * 	WMB				RMB
> -	 * 	write INT3			if (in_progress)
> +	 *	in_progress = TRUE		INT3
> +	 *	WMB				RMB
> +	 *	write INT3			if (in_progress)

I don’t see what has changed in this chunk… Whitespaces?

> 	 *
> -	 * Idem for bp_int3_handler.
> +	 * Idem for bp_int3_opcode.
> 	 */
> 	smp_rmb();
> 
> @@ -943,8 +949,21 @@ int poke_int3_handler(struct pt_regs *re
> 	if (user_mode(regs) || regs->ip != (unsigned long)bp_int3_addr)
> 		return 0;
> 
> -	/* set up the specified breakpoint handler */
> -	regs->ip = (unsigned long) bp_int3_handler;
> +	opcode = *(struct opcode *)bp_int3_opcode;
> +
> +	switch (opcode.insn) {
> +	case 0xE8: /* CALL */
> +		int3_emulate_call(regs, ip + opcode.rel);
> +		break;
> +
> +	case 0xE9: /* JMP */
> +		int3_emulate_jmp(regs, ip + opcode.rel);
> +		break;

Consider using RELATIVECALL_OPCODE and RELATIVEJUMP_OPCODE instead of the
constants (0xE8, 0xE9), just as you do later in the patch.