Messages in this thread | | | Date | Mon, 5 Jul 2021 10:34:58 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH -tip v8 13/13] x86/kprobes: Fixup return address in generic trampoline handler |
| |
* Masami Hiramatsu <mhiramat@kernel.org> wrote:
> In x86, kretprobe trampoline address on the stack frame will > be replaced with the real return address after returning from > trampoline_handler. Before fixing the return address, the real > return address can be found in the current->kretprobe_instances. > > However, since there is a window between updating the > current->kretprobe_instances and fixing the address on the stack, > if an interrupt caused at that timing and the interrupt handler > does stacktrace, it may fail to unwind because it can not get > the correct return address from current->kretprobe_instances. > > This will minimize that window by fixing the return address > right before updating current->kretprobe_instances.
Is there still a window? I.e. is it "minimized" (to how big of a window?), or eliminated?
> +void arch_kretprobe_fixup_return(struct pt_regs *regs, > + unsigned long correct_ret_addr) > +{ > + unsigned long *frame_pointer; > + > + frame_pointer = ((unsigned long *)®s->sp) + 1; > + > + /* Replace fake return address with real one. */ > + *frame_pointer = correct_ret_addr;
Firstly, why does ®s->sp have to be forced to 'unsigned long *'?
pt_regs::sp is 'unsigned long' on both 32-bit and 64-bit kernels AFAICS.
Secondly, the new code modified by your patch now looks like this:
frame_pointer = ((unsigned long *)®s->sp) + 1;
+ kretprobe_trampoline_handler(regs, frame_pointer);
where:
+void arch_kretprobe_fixup_return(struct pt_regs *regs, + unsigned long correct_ret_addr) +{ + unsigned long *frame_pointer; + + frame_pointer = ((unsigned long *)®s->sp) + 1; + + /* Replace fake return address with real one. */ + *frame_pointer = correct_ret_addr; +}
So we first do:
frame_pointer = ((unsigned long *)®s->sp) + 1;
... and pass that in to arch_kretprobe_fixup_return() as 'correct_ret_addr', which does:
+ frame_pointer = ((unsigned long *)®s->sp) + 1; + *frame_pointer = correct_ret_addr;
... which looks like the exact same thing as:
*frame_pointer = frame_pointer;
... obfuscated through a thick layer of type casts?
Thanks,
Ingo
| |