linux-kernel - Re: [for-next][PATCH 04/18] x86/ftrace: Do not jump to direct code in created trampolines

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Jul 2020 23:24:59 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [for-next][PATCH 04/18] x86/ftrace: Do not jump to direct code
 in created trampolines

On Fri, 3 Jul 2020 10:10:00 +0200
Peter Zijlstra <peterz@...radead.org> wrote:

> On Thu, Jul 02, 2020 at 05:58:16PM -0400, Steven Rostedt wrote:
> 
> > +	/* No need to test direct calls on created trampolines */
> > +	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > +		/* NOP the jnz 1f; but make sure it's a 2 byte jnz */
> > +		ip = trampoline + (jmp_offset - start_offset);
> > +		if (WARN_ON(*(char *)ip != 0x75))
> > +			goto fail;
> > +		ret = copy_from_kernel_nofault(ip, ideal_nops[2], 2);  
> 
> I really don't get this paranoia, what's wrong with memcpy() ?

Habit. As when ftrace was introduced, it was extremely careful about
touching memory like this. And even with all of that extra care, we
still broke NICs (actually, some of the reason those NICs broke was
because of the extra care we took :-p)

> 
> > +		if (ret < 0)
> > +			goto fail;
> > +	}  
> 
> How about something like this?
> 
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -359,17 +359,11 @@ create_trampoline(struct ftrace_ops *ops
>  	npages = DIV_ROUND_UP(*tramp_size, PAGE_SIZE);
>  
>  	/* Copy ftrace_caller onto the trampoline memory */
> -	ret = copy_from_kernel_nofault(trampoline, (void *)start_offset, size);
> -	if (WARN_ON(ret < 0))
> -		goto fail;
> -
> -	ip = trampoline + size;
> +	memcpy(trampoline, (void *)start_offset, size);
>  
>  	/* The trampoline ends with ret(q) */
> -	retq = (unsigned long)ftrace_stub;
> -	ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE);
> -	if (WARN_ON(ret < 0))
> -		goto fail;
> +	ip = trampoline + size;
> +	memcpy(ip, text_gen_insn(RET_INSN_OPCODE, NULL, NULL), RET_INSN_SIZE);
>  
>  	/* No need to test direct calls on created trampolines */
>  	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> @@ -377,9 +371,7 @@ create_trampoline(struct ftrace_ops *ops
>  		ip = trampoline + (jmp_offset - start_offset);
>  		if (WARN_ON(*(char *)ip != 0x75))
>  			goto fail;
> -		ret = copy_from_kernel_nofault(ip, ideal_nops[2], 2);
> -		if (ret < 0)
> -			goto fail;
> +		memcpy(ip, ideal_nops[2], 2);

If you want to add this change on top of this, then I'm fine with that.

If it breaks something, I can at least point the blame at you ;-)

-- Steve


>  	}
>  
>  	/*