| Message ID | 20220325114012.GO8939@worktop.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | kprobes: rethook: x86: Replace kretprobe trampoline with rethook | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | fail | merge-conflict |
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 = ®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;
--- 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;