Re: [PATCH v2 00/13] objtool: Detect and warn about indirect calls in __nocfi functions
From: Josh Poimboeuf <jpoimboe@kernel.org>
Date: 2025-06-05 17:19:44
Also in:
kvm, linux-efi, lkml
Subsystem:
intel(r) flexible return and event delivery, the rest, x86 architecture (32-bit and 64-bit), x86 entry code · Maintainers:
Xin Li, "H. Peter Anvin", Linus Torvalds, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, Andy Lutomirski
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:quoted
On Thu, May 29, 2025 at 11:30:17AM +0200, Peter Zijlstra wrote:quoted
quoted
quoted
So the sequence of fail is: push %rbp mov %rsp, %rbp # cfa.base = BP SAVEsub $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);