linux-kernel - Re: [PATCH 09/10] ftrace/x86: Add register_ftrace

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]

Date:   Thu, 14 Nov 2019 16:34:58 +0100 (CET)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Steven Rostedt <rostedt@...dmis.org>
cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        X86 ML <x86@...nel.org>, Nadav Amit <nadav.amit@...il.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Song Liu <songliubraving@...com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH 09/10] ftrace/x86: Add register_ftrace_direct() for custom
 trampolines

On Fri, 8 Nov 2019, Steven Rostedt wrote:

> From: "Steven Rostedt (VMware)" <rostedt@...dmis.org>
> 
> Enable x86 to allow for register_ftrace_direct(), where a custom trampoline
> may be called directly from an ftrace mcount/fentry location.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>

[...]

> +++ b/arch/x86/kernel/ftrace_64.S
> @@ -88,6 +88,7 @@ EXPORT_SYMBOL(__fentry__)
>  	movq %rdi, RDI(%rsp)
>  	movq %r8, R8(%rsp)
>  	movq %r9, R9(%rsp)
> +	movq $0, ORIG_RAX(%rsp)
>  	/*
>  	 * Save the original RBP. Even though the mcount ABI does not
>  	 * require this, it helps out callers.
> @@ -114,7 +115,8 @@ EXPORT_SYMBOL(__fentry__)
>  	subq $MCOUNT_INSN_SIZE, %rdi
>  	.endm
>  
> -.macro restore_mcount_regs
> +.macro restore_mcount_regs save=0
> +
>  	movq R9(%rsp), %r9
>  	movq R8(%rsp), %r8
>  	movq RDI(%rsp), %rdi
> @@ -123,10 +125,7 @@ EXPORT_SYMBOL(__fentry__)
>  	movq RCX(%rsp), %rcx
>  	movq RAX(%rsp), %rax
>  
> -	/* ftrace_regs_caller can modify %rbp */
> -	movq RBP(%rsp), %rbp
> -
> -	addq $MCOUNT_REG_SIZE, %rsp
> +	addq $MCOUNT_REG_SIZE-\save, %rsp
>  
>  	.endm
>  
> @@ -228,10 +227,30 @@ GLOBAL(ftrace_regs_call)
>  	movq R10(%rsp), %r10
>  	movq RBX(%rsp), %rbx
>  
> -	restore_mcount_regs
> +	movq RBP(%rsp), %rbp
> +
> +	movq ORIG_RAX(%rsp), %rax
> +	movq %rax, MCOUNT_REG_SIZE-8(%rsp)
> +
> +	/* If ORIG_RAX is anything but zero, make this a call to that */
> +	movq ORIG_RAX(%rsp), %rax
> +	cmpq	$0, %rax
> +	je	1f
> +
> +	/* Swap the flags with orig_rax */
> +	movq MCOUNT_REG_SIZE(%rsp), %rdi
> +	movq %rdi, MCOUNT_REG_SIZE-8(%rsp)
> +	movq %rax, MCOUNT_REG_SIZE(%rsp)
> +
> +	restore_mcount_regs 8
> +
> +	jmp	2f
> +
> +1:	restore_mcount_regs
> +
>  
>  	/* Restore flags */
> -	popfq
> +2:	popfq

If I am reading the code correctly (and I was confused couple of times, so 
maybe I am not), this is what makes the direct fops incompatible with 
ipmodify and livepatching for now. Is that correct?

What are your plans regarding this?

Moreover, we could replace ftrace_regs_caller with direct fops for live 
patching when this is merged with all arch support we need. After all, all 
we need is to change the rip, which we could do easily in the direct 
trampoline. On the other hand, it would exclude coexistence of a live 
patch and a BPF filter (both direct now) on one function.

I may have missed something though.

Thanks
Miroslav