From c4239a72a29d58a4377c2db8583c24f9e2b79d01 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Fri, 7 Feb 2025 13:15:37 +0100 Subject: [PATCH] x86/ibt: Clean up poison_endbr() Basically, get rid of the .warn argument and explicitly don't call the function when we know there isn't an endbr. This makes the calling code clearer. Note: perhaps don't add functions to .cfi_sites when the function doesn't have endbr -- OTOH why would the compiler emit the prefix if it has already determined there are no indirect callers and has omitted the ENDBR instruction. Signed-off-by: Peter Zijlstra (Intel) Reviewed-by: Sami Tolvanen Link: https://lore.kernel.org/r/20250207122546.815505775@infradead.org --- arch/x86/kernel/alternative.c | 43 +++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index fda11df1e6501..e285933506e99 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -865,14 +865,12 @@ Efault: static void poison_cfi(void *addr); -static void __init_or_module poison_endbr(void *addr, bool warn) +static void __init_or_module poison_endbr(void *addr) { u32 poison = gen_endbr_poison(); - if (!is_endbr(addr)) { - WARN_ON_ONCE(warn); + if (WARN_ON_ONCE(!is_endbr(addr))) return; - } DPRINTK(ENDBR, "ENDBR at: %pS (%px)", addr, addr); @@ -897,7 +895,7 @@ void __init_or_module noinline apply_seal_endbr(s32 *start, s32 *end) for (s = start; s < end; s++) { void *addr = (void *)s + *s; - poison_endbr(addr, true); + poison_endbr(addr); if (IS_ENABLED(CONFIG_FINEIBT)) poison_cfi(addr - 16); } @@ -1200,6 +1198,14 @@ static int cfi_rewrite_preamble(s32 *start, s32 *end) void *addr = (void *)s + *s; u32 hash; + /* + * When the function doesn't start with ENDBR the compiler will + * have determined there are no indirect calls to it and we + * don't need no CFI either. + */ + if (!is_endbr(addr + 16)) + continue; + hash = decode_preamble_hash(addr); if (WARN(!hash, "no CFI hash found at: %pS %px %*ph\n", addr, addr, 5, addr)) @@ -1220,7 +1226,10 @@ static void cfi_rewrite_endbr(s32 *start, s32 *end) for (s = start; s < end; s++) { void *addr = (void *)s + *s; - poison_endbr(addr+16, false); + if (!is_endbr(addr + 16)) + continue; + + poison_endbr(addr + 16); } } @@ -1353,8 +1362,22 @@ static inline void poison_hash(void *addr) static void poison_cfi(void *addr) { + /* + * Compilers manage to be inconsistent with ENDBR vs __cfi prefixes, + * some (static) functions for which they can determine the address + * is never taken do not get a __cfi prefix, but *DO* get an ENDBR. + * + * As such, these functions will get sealed, but we need to be careful + * to not unconditionally scribble the previous function. + */ switch (cfi_mode) { case CFI_FINEIBT: + /* + * FineIBT prefix should start with an ENDBR. + */ + if (!is_endbr(addr)) + break; + /* * __cfi_\func: * osp nopl (%rax) @@ -1363,11 +1386,17 @@ static void poison_cfi(void *addr) * ud2 * 1: nop */ - poison_endbr(addr, false); + poison_endbr(addr); poison_hash(addr + fineibt_preamble_hash); break; case CFI_KCFI: + /* + * kCFI prefix should start with a valid hash. + */ + if (!decode_preamble_hash(addr)) + break; + /* * __cfi_\func: * movl $0, %eax -- 2.47.2