linux-kernel - Re: [PATCH -tip v8 13/13] x86/kprobes: Fixup return address in generic trampoline handler

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 5 Jul 2021 10:34:58 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>, X86 ML <x86@...nel.org>,
        Daniel Xu <dxu@...uu.xyz>, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org, kuba@...nel.org, mingo@...hat.com,
        ast@...nel.org, Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        Peter Zijlstra <peterz@...radead.org>, kernel-team@...com,
        yhs@...com, linux-ia64@...r.kernel.org,
        Abhishek Sagar <sagar.abhishek@...il.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>
Subject: Re: [PATCH -tip v8 13/13] x86/kprobes: Fixup return address in
 generic trampoline handler


* Masami Hiramatsu <mhiramat@...nel.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 *)&regs->sp) + 1;
> +
> +	/* Replace fake return address with real one. */
> +	*frame_pointer = correct_ret_addr;

Firstly, why does &regs->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 *)&regs->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 *)&regs->sp) + 1;
+
+       /* Replace fake return address with real one. */
+       *frame_pointer = correct_ret_addr;
+}

So we first do:

        frame_pointer = ((unsigned long *)&regs->sp) + 1;

... and pass that in to arch_kretprobe_fixup_return() as 
'correct_ret_addr', which does:

+       frame_pointer = ((unsigned long *)&regs->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