Re: [PATCH bpf-next 3/2] x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs

From: Masami Hiramatsu <mhiramat@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	x86@kernel.org, Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	kernel-janitors@vger.kernel.org,
	Steven Rostedt <rostedt@goodmis.org>,
	Jiri Olsa <jolsa@kernel.org>,
	bpf@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next 3/2] x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs
Date: Fri, 25 Mar 2022 22:01:13 +0900	[thread overview]
Message-ID: <20220325220113.a80c05905b7633b105a7abe1@kernel.org> (raw)
In-Reply-To: <20220325114012.GO8939@worktop.programming.kicks-ass.net>

On Fri, 25 Mar 2022 12:40:12 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> 
> You lost the regs->ss bit again..

Yeah, I planed to split it in another series with optprobe update
because the optprobe also skips regs->ss. Anyway...

> 
> Boot tested on tigerlake with IBT enabled -- passed the boot time
> kretprobe selftests.
> 
> ---
> 
> Subject: x86,rethook: Fix arch_rethook_trampoline() to generate a complete pt_regs
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Fri Mar 25 10:25:56 CET 2022
> 
> Currently arch_rethook_trampoline() generates an almost complete
> pt_regs on-stack, everything except regs->ss that is, that currently
> points to the fake return address, which is not a valid segment
> descriptor.
> 
> Since interpretation of regs->[sb]p should be done in the context of
> regs->ss, and we have code actually doing that (see
> arch/x86/lib/insn-eval.c for instance), complete the job by also
> pushing ss.

This looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> 
> This ensures that anybody who does do look at regs->ss doesn't
> mysteriously malfunction, avoiding much future pain.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/kernel/rethook.c |   24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> --- a/arch/x86/kernel/rethook.c
> +++ b/arch/x86/kernel/rethook.c
> @@ -25,29 +25,31 @@ asm(
>  	/* Push a fake return address to tell the unwinder it's a kretprobe. */
>  	"	pushq $arch_rethook_trampoline\n"
>  	UNWIND_HINT_FUNC
> -	/* Save the 'sp - 8', this will be fixed later. */
> +	"       pushq $" __stringify(__KERNEL_DS) "\n"
> +	/* Save the 'sp - 16', this will be fixed later. */
>  	"	pushq %rsp\n"
>  	"	pushfq\n"
>  	SAVE_REGS_STRING
>  	"	movq %rsp, %rdi\n"
>  	"	call arch_rethook_trampoline_callback\n"
>  	RESTORE_REGS_STRING
> -	/* In the callback function, 'regs->flags' is copied to 'regs->sp'. */
> -	"	addq $8, %rsp\n"
> +	/* In the callback function, 'regs->flags' is copied to 'regs->ss'. */
> +	"	addq $16, %rsp\n"
>  	"	popfq\n"
>  #else
>  	/* Push a fake return address to tell the unwinder it's a kretprobe. */
>  	"	pushl $arch_rethook_trampoline\n"
>  	UNWIND_HINT_FUNC
> -	/* Save the 'sp - 4', this will be fixed later. */
> +	"	pushl %ss\n"
> +	/* Save the 'sp - 8', this will be fixed later. */
>  	"	pushl %esp\n"
>  	"	pushfl\n"
>  	SAVE_REGS_STRING
>  	"	movl %esp, %eax\n"
>  	"	call arch_rethook_trampoline_callback\n"
>  	RESTORE_REGS_STRING
> -	/* In the callback function, 'regs->flags' is copied to 'regs->sp'. */
> -	"	addl $4, %esp\n"
> +	/* In the callback function, 'regs->flags' is copied to 'regs->ss'. */
> +	"	addl $8, %esp\n"
>  	"	popfl\n"
>  #endif
>  	ASM_RET
> @@ -69,8 +71,8 @@ __used __visible void arch_rethook_tramp
>  #endif
>  	regs->ip = (unsigned long)&arch_rethook_trampoline;
>  	regs->orig_ax = ~0UL;
> -	regs->sp += sizeof(long);
> -	frame_pointer = &regs->sp + 1;
> +	regs->sp += 2*sizeof(long);
> +	frame_pointer = (long *)(regs + 1);
>  
>  	/*
>  	 * The return address at 'frame_pointer' is recovered by the
> @@ -80,10 +82,10 @@ __used __visible void arch_rethook_tramp
>  	rethook_trampoline_handler(regs, (unsigned long)frame_pointer);
>  
>  	/*
> -	 * Copy FLAGS to 'pt_regs::sp' so that arch_rethook_trapmoline()
> +	 * Copy FLAGS to 'pt_regs::ss' so that arch_rethook_trapmoline()
>  	 * can do RET right after POPF.
>  	 */
> -	regs->sp = regs->flags;
> +	*(unsigned long *)&regs->ss = regs->flags;
>  }
>  NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
>  
> @@ -101,7 +103,7 @@ STACK_FRAME_NON_STANDARD_FP(arch_rethook
>  void arch_rethook_fixup_return(struct pt_regs *regs,
>  			       unsigned long correct_ret_addr)
>  {
> -	unsigned long *frame_pointer = &regs->sp + 1;
> +	unsigned long *frame_pointer = (void *)(regs + 1);
>  
>  	/* Replace fake return address with real one. */
>  	*frame_pointer = correct_ret_addr;

-- 
Masami Hiramatsu <mhiramat@kernel.org>