On Fri, 25 Mar 2022 12:40:12 +0100
Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxx>
> ---
> 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 = ®s->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 *)®s->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 = ®s->sp + 1;
> + unsigned long *frame_pointer = (void *)(regs + 1);
>
> /* Replace fake return address with real one. */
> *frame_pointer = correct_ret_addr;
--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>
