linux-kernel - Re: [PATCH v2 00/13] objtool: Detect and warn about indirect calls in _

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 for Android: free password hash cracker in your pocket

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

Message-ID: <4z4fhaqesjlevwiugiqpnxdths5qkkj7vd4q3wgdosu4p24ppl@nb6c2gybuwe5>
Date: Thu, 5 Jun 2025 10:19:41 -0700
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Sean Christopherson <seanjc@...gle.com>, 
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org, kys@...rosoft.com, haiyangz@...rosoft.com, 
	wei.liu@...nel.org, decui@...rosoft.com, tglx@...utronix.de, mingo@...hat.com, 
	bp@...en8.de, dave.hansen@...ux.intel.com, pbonzini@...hat.com, 
	ardb@...nel.org, kees@...nel.org, Arnd Bergmann <arnd@...db.de>, 
	gregkh@...uxfoundation.org, linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org, 
	kvm@...r.kernel.org, linux-efi@...r.kernel.org, samitolvanen@...gle.com, 
	ojeda@...nel.org, xin@...or.com
Subject: Re: [PATCH v2 00/13] objtool: Detect and warn about indirect calls
 in __nocfi functions

On Tue, Jun 03, 2025 at 09:29:45AM -0700, Josh Poimboeuf wrote:
> On Mon, Jun 02, 2025 at 10:43:42PM -0700, Josh Poimboeuf wrote:
> > On Thu, May 29, 2025 at 11:30:17AM +0200, Peter Zijlstra wrote:
> > > > > So the sequence of fail is:
> > > > > 
> > > > > 	push %rbp
> > > > > 	mov %rsp, %rbp	# cfa.base = BP
> > > > > 
> > > > > 	SAVE
> > > 
> > > 	sub    $0x40,%rsp
> > > 	and    $0xffffffffffffffc0,%rsp
> > > 
> > > This hits the 'older GCC, drap with frame pointer' case in OP_SRC_AND.
> > > Which means we then hard rely on the frame pointer to get things right.
> > > 
> > > However, per all the PUSH/POP_REGS nonsense, BP can get clobbered.
> > > Specifically the code between the CALL and POP %rbp below are up in the
> > > air. I don't think it can currently unwind properly there.
> > 
> > RBP is callee saved, so there's no need to pop it or any of the other
> > callee-saved regs.  If they were to change, that would break C ABI
> > pretty badly.  Maybe add a skip_callee=1 arg to POP_REGS?
> 
> This compiles for me:

That last patch had a pretty heinous bug: it didn't adjust the stack
accordingly when it skipped the callee-saved pops.

But actually there's no need to pop *any* regs there.

asm_fred_entry_from_kvm() uses C function ABI, so changes to
callee-saved regs aren't allowed, and changes to caller-saved regs would
have no effect.

How about something like this?

diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index d83236b96f22..e680afbf65b6 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -99,7 +99,7 @@ For 32-bit we have the following conventions - kernel is built with
 	.endif
 .endm
 
-.macro CLEAR_REGS clear_bp=1
+.macro CLEAR_REGS clear_callee=1
 	/*
 	 * Sanitize registers of values that a speculation attack might
 	 * otherwise want to exploit. The lower registers are likely clobbered
@@ -113,20 +113,19 @@ For 32-bit we have the following conventions - kernel is built with
 	xorl	%r9d,  %r9d	/* nospec r9  */
 	xorl	%r10d, %r10d	/* nospec r10 */
 	xorl	%r11d, %r11d	/* nospec r11 */
+	.if \clear_callee
 	xorl	%ebx,  %ebx	/* nospec rbx */
-	.if \clear_bp
 	xorl	%ebp,  %ebp	/* nospec rbp */
-	.endif
 	xorl	%r12d, %r12d	/* nospec r12 */
 	xorl	%r13d, %r13d	/* nospec r13 */
 	xorl	%r14d, %r14d	/* nospec r14 */
 	xorl	%r15d, %r15d	/* nospec r15 */
-
+	.endif
 .endm
 
-.macro PUSH_AND_CLEAR_REGS rdx=%rdx rcx=%rcx rax=%rax save_ret=0 clear_bp=1 unwind_hint=1
+.macro PUSH_AND_CLEAR_REGS rdx=%rdx rcx=%rcx rax=%rax save_ret=0 clear_callee=1 unwind_hint=1
 	PUSH_REGS rdx=\rdx, rcx=\rcx, rax=\rax, save_ret=\save_ret unwind_hint=\unwind_hint
-	CLEAR_REGS clear_bp=\clear_bp
+	CLEAR_REGS clear_callee=\clear_callee
 .endm
 
 .macro POP_REGS pop_rdi=1
diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
index 29c5c32c16c3..5d1eef193b79 100644
--- a/arch/x86/entry/entry_64_fred.S
+++ b/arch/x86/entry/entry_64_fred.S
@@ -112,11 +112,12 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
 	push %rax				/* Return RIP */
 	push $0					/* Error code, 0 for IRQ/NMI */
 
-	PUSH_AND_CLEAR_REGS clear_bp=0 unwind_hint=0
+	PUSH_AND_CLEAR_REGS clear_callee=0 unwind_hint=0
 	movq %rsp, %rdi				/* %rdi -> pt_regs */
 	call __fred_entry_from_kvm		/* Call the C entry point */
-	POP_REGS
-	ERETS
+	addq $C_PTREGS_SIZE, %rsp
+
+	ALTERNATIVE "mov %rbp, %rsp", __stringify(ERETS), X86_FEATURE_FRED
 1:
 	/*
 	 * Objtool doesn't understand what ERETS does, this hint tells it that
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index ad4ea6fb3b6c..d4f9bfdc24a7 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -94,6 +94,7 @@ static void __used common(void)
 
 	BLANK();
 	DEFINE(PTREGS_SIZE, sizeof(struct pt_regs));
+	OFFSET(C_PTREGS_SIZE, pt_regs, orig_ax);
 
 	/* TLB state for the entry code */
 	OFFSET(TLB_STATE_user_pcid_flush_mask, tlb_state, user_pcid_flush_mask);