From nobody Thu Dec 18 11:31:34 2025 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B91521EE7AD for ; Mon, 24 Mar 2025 21:56:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1742853383; cv=none; b=O3FZTvAPJ8ElZ1PIKddfWSflQPt3YoO+FkNOw9K48v86Qjnti1tV4knZcSFhgAgLegPeNu00Sn5g4tBYDeXgtqsqOPPQJVeBxXa1iGg/VKZCscp6OBiOuovcLP0hTCxOeV/0gFtjaJKq8Lo4KYDUQfC2RGBdUnvP1zwsQHI04Tg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1742853383; c=relaxed/simple; bh=oKg4PMp9YXn1Ng8aPqaH+AE4nP+pk0Yy0+LvRr0vDWY=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=SQVQiTmgzdtbVxfMM1XkNEFZphJ0rfgRoMdQQP5jC9udPy5/VS5II4ME02fabZjImmPkg89vMZ+Xsl+xKLgpTmdXI12EyIsFytyRsW8RCEu6dJ+MbjUdcBKHSO3pcZqTieW/XFDo79vIs8UjKr/Uk3PzMqsPJ7RTprDN15JPbDA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=lZpw+BNu; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="lZpw+BNu" Received: by smtp.kernel.org (Postfix) with ESMTPSA id DAA62C4CEE4; Mon, 24 Mar 2025 21:56:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1742853383; bh=oKg4PMp9YXn1Ng8aPqaH+AE4nP+pk0Yy0+LvRr0vDWY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=lZpw+BNu+o6hCS8+d1ZnVk76hdtzoZ7P2y6rz+SoLVNJmC1LKQQz0tmfOrzYxRT0r BBBcDb2sn4pvYh+ytPrmLTCnyk6lCjWYPgVa58v9YlY5EJdr5DRYxWXDLxCzcU3s0J wUiw4qDRFHlqNEVnuf6ITY4wir5/v9nTkwj9kz9c6Fv9HLSF3Nai8O4v9kKeSZM0Or WSdzm8MYuGc+XKROcG6LMb3H8VuHyGZA+sJ8YNgA+ytCvUpRzTeuEkfqWv9yTTI92u KeRG8Ym+Mf/0F/u7z0bFAXtjNpMj90K+zIgXCcGTKlaa4Qf6GSS0pG16ZD1qDDxXEZ KLh8te6ihMjeg== From: Josh Poimboeuf To: x86@kernel.org Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , Ingo Molnar , Miroslav Benes , Brendan Jackman , Nathan Chancellor , kernel test robot Subject: [PATCH 04/22] objtool: Fix X86_FEATURE_SMAP alternative handling Date: Mon, 24 Mar 2025 14:55:54 -0700 Message-ID: X-Mailer: git-send-email 2.48.1 In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" For X86_FEATURE_SMAP alternatives which replace NOP with STAC or CLAC, uaccess validation skips the NOP branch to avoid following impossible code paths, e.g. where a STAC would be patched but a CLAC wouldn't. However, it's not safe to assume an X86_FEATURE_SMAP alternative is patching STAC/CLAC. There can be other alternatives, like static_cpu_has(), where both branches need to be validated. Fix that by repurposing ANNOTATE_IGNORE_ALTERNATIVE for skipping either original instructions or new ones. This is a more generic approach which enables the removal of the feature checking hacks and the insn->ignore bit. Fixes the following warnings: arch/x86/mm/fault.o: warning: objtool: do_user_addr_fault+0x8ec: __stack_= chk_fail() missing __noreturn in .c/.h or NORETURN() in noreturns.h arch/x86/mm/fault.o: warning: objtool: do_user_addr_fault+0x8f1: unreacha= ble instruction Fixes: ea24213d8088 ("objtool: Add UACCESS validation") Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202503181736.zkZUBv4N-lkp@int= el.com/ Signed-off-by: Josh Poimboeuf --- arch/x86/include/asm/arch_hweight.h | 8 +++-- arch/x86/include/asm/smap.h | 23 ++++++++++----- arch/x86/include/asm/xen/hypercall.h | 6 ++-- tools/objtool/arch/x86/special.c | 33 +-------------------- tools/objtool/check.c | 39 +++++++------------------ tools/objtool/include/objtool/check.h | 3 +- tools/objtool/include/objtool/special.h | 4 +-- tools/objtool/special.c | 12 ++------ 8 files changed, 39 insertions(+), 89 deletions(-) diff --git a/arch/x86/include/asm/arch_hweight.h b/arch/x86/include/asm/arc= h_hweight.h index ba88edd0d58b..a0eaa8fda55f 100644 --- a/arch/x86/include/asm/arch_hweight.h +++ b/arch/x86/include/asm/arch_hweight.h @@ -16,7 +16,9 @@ static __always_inline unsigned int __arch_hweight32(unsi= gned int w) { unsigned int res; =20 - asm (ALTERNATIVE("call __sw_hweight32", "popcntl %1, %0", X86_FEATURE_POP= CNT) + asm (ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE + "call __sw_hweight32", + "popcntl %1, %0", X86_FEATURE_POPCNT) : "=3D"REG_OUT (res) : REG_IN (w)); =20 @@ -44,7 +46,9 @@ static __always_inline unsigned long __arch_hweight64(__u= 64 w) { unsigned long res; =20 - asm (ALTERNATIVE("call __sw_hweight64", "popcntq %1, %0", X86_FEATURE_POP= CNT) + asm (ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE + "call __sw_hweight64", + "popcntq %1, %0", X86_FEATURE_POPCNT) : "=3D"REG_OUT (res) : REG_IN (w)); =20 diff --git a/arch/x86/include/asm/smap.h b/arch/x86/include/asm/smap.h index 2de1e5a75c57..a984cdd6de30 100644 --- a/arch/x86/include/asm/smap.h +++ b/arch/x86/include/asm/smap.h @@ -16,23 +16,23 @@ #ifdef __ASSEMBLY__ =20 #define ASM_CLAC \ - ALTERNATIVE "", "clac", X86_FEATURE_SMAP + ALTERNATIVE __stringify(ANNOTATE_IGNORE_ALTERNATIVE), "clac", X86_FEATURE= _SMAP =20 #define ASM_STAC \ - ALTERNATIVE "", "stac", X86_FEATURE_SMAP + ALTERNATIVE __stringify(ANNOTATE_IGNORE_ALTERNATIVE), "stac", X86_FEATURE= _SMAP =20 #else /* __ASSEMBLY__ */ =20 static __always_inline void clac(void) { /* Note: a barrier is implicit in alternative() */ - alternative("", "clac", X86_FEATURE_SMAP); + alternative(ANNOTATE_IGNORE_ALTERNATIVE "", "clac", X86_FEATURE_SMAP); } =20 static __always_inline void stac(void) { /* Note: a barrier is implicit in alternative() */ - alternative("", "stac", X86_FEATURE_SMAP); + alternative(ANNOTATE_IGNORE_ALTERNATIVE "", "stac", X86_FEATURE_SMAP); } =20 static __always_inline unsigned long smap_save(void) @@ -40,7 +40,8 @@ static __always_inline unsigned long smap_save(void) unsigned long flags; =20 asm volatile ("# smap_save\n\t" - ALTERNATIVE("", "pushf; pop %0; " "clac" "\n\t", + ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE + "", "pushf; pop %0; clac", X86_FEATURE_SMAP) : "=3Drm" (flags) : : "memory", "cc"); =20 @@ -50,16 +51,22 @@ static __always_inline unsigned long smap_save(void) static __always_inline void smap_restore(unsigned long flags) { asm volatile ("# smap_restore\n\t" - ALTERNATIVE("", "push %0; popf\n\t", + ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE + "", "push %0; popf", X86_FEATURE_SMAP) : : "g" (flags) : "memory", "cc"); } =20 /* These macros can be used in asm() statements */ #define ASM_CLAC \ - ALTERNATIVE("", "clac", X86_FEATURE_SMAP) + ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE "", "clac", X86_FEATURE_SMAP) #define ASM_STAC \ - ALTERNATIVE("", "stac", X86_FEATURE_SMAP) + ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE "", "stac", X86_FEATURE_SMAP) + +#define ASM_CLAC_UNSAFE \ + ALTERNATIVE("", ANNOTATE_IGNORE_ALTERNATIVE "clac", X86_FEATURE_SMAP) +#define ASM_STAC_UNSAFE \ + ALTERNATIVE("", ANNOTATE_IGNORE_ALTERNATIVE "stac", X86_FEATURE_SMAP) =20 #endif /* __ASSEMBLY__ */ =20 diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xe= n/hypercall.h index 97771b9d33af..59a62c3780a2 100644 --- a/arch/x86/include/asm/xen/hypercall.h +++ b/arch/x86/include/asm/xen/hypercall.h @@ -231,14 +231,12 @@ static __always_inline void __xen_stac(void) * Suppress objtool seeing the STAC/CLAC and getting confused about it * calling random code with AC=3D1. */ - asm volatile(ANNOTATE_IGNORE_ALTERNATIVE - ASM_STAC ::: "memory", "flags"); + asm volatile(ASM_STAC_UNSAFE ::: "memory", "flags"); } =20 static __always_inline void __xen_clac(void) { - asm volatile(ANNOTATE_IGNORE_ALTERNATIVE - ASM_CLAC ::: "memory", "flags"); + asm volatile(ASM_CLAC_UNSAFE ::: "memory", "flags"); } =20 static inline long diff --git a/tools/objtool/arch/x86/special.c b/tools/objtool/arch/x86/spec= ial.c index 5f46d4e7f7f8..403e587676f1 100644 --- a/tools/objtool/arch/x86/special.c +++ b/tools/objtool/arch/x86/special.c @@ -5,10 +5,7 @@ #include #include =20 -#define X86_FEATURE_POPCNT (4 * 32 + 23) -#define X86_FEATURE_SMAP (9 * 32 + 20) - -void arch_handle_alternative(unsigned short feature, struct special_alt *a= lt) +void arch_handle_alternative(struct special_alt *alt) { static struct special_alt *group, *prev; =20 @@ -32,34 +29,6 @@ void arch_handle_alternative(unsigned short feature, str= uct special_alt *alt) } else group =3D alt; =20 prev =3D alt; - - switch (feature) { - case X86_FEATURE_SMAP: - /* - * If UACCESS validation is enabled; force that alternative; - * otherwise force it the other way. - * - * What we want to avoid is having both the original and the - * alternative code flow at the same time, in that case we can - * find paths that see the STAC but take the NOP instead of - * CLAC and the other way around. - */ - if (opts.uaccess) - alt->skip_orig =3D true; - else - alt->skip_alt =3D true; - break; - case X86_FEATURE_POPCNT: - /* - * It has been requested that we don't validate the !POPCNT - * feature path which is a "very very small percentage of - * machines". - */ - alt->skip_orig =3D true; - break; - default: - break; - } } =20 bool arch_support_alt_relocation(struct special_alt *special_alt, diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 8e2f3405d78c..bac88ca82dec 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -25,7 +25,6 @@ struct alternative { struct alternative *next; struct instruction *insn; - bool skip_orig; }; =20 static unsigned long nr_cfi, nr_cfi_reused, nr_cfi_cache; @@ -1704,6 +1703,7 @@ static int handle_group_alt(struct objtool_file *file, orig_alt_group->first_insn =3D orig_insn; orig_alt_group->last_insn =3D last_orig_insn; orig_alt_group->nop =3D NULL; + orig_alt_group->ignore =3D orig_insn->ignore_alts; } else { if (orig_alt_group->last_insn->offset + orig_alt_group->last_insn->len - orig_alt_group->first_insn->offset !=3D special_alt->orig_len) { @@ -1743,7 +1743,6 @@ static int handle_group_alt(struct objtool_file *file, nop->type =3D INSN_NOP; nop->sym =3D orig_insn->sym; nop->alt_group =3D new_alt_group; - nop->ignore =3D orig_insn->ignore_alts; } =20 if (!special_alt->new_len) { @@ -1760,7 +1759,6 @@ static int handle_group_alt(struct objtool_file *file, =20 last_new_insn =3D insn; =20 - insn->ignore =3D orig_insn->ignore_alts; insn->sym =3D orig_insn->sym; insn->alt_group =3D new_alt_group; =20 @@ -1807,6 +1805,7 @@ static int handle_group_alt(struct objtool_file *file, new_alt_group->first_insn =3D *new_insn; new_alt_group->last_insn =3D last_new_insn; new_alt_group->nop =3D nop; + new_alt_group->ignore =3D (*new_insn)->ignore_alts; new_alt_group->cfi =3D orig_alt_group->cfi; return 0; } @@ -1924,8 +1923,6 @@ static int add_special_section_alts(struct objtool_fi= le *file) } =20 alt->insn =3D new_insn; - alt->skip_orig =3D special_alt->skip_orig; - orig_insn->ignore_alts |=3D special_alt->skip_alt; alt->next =3D orig_insn->alts; orig_insn->alts =3D alt; =20 @@ -2303,6 +2300,8 @@ static int read_annotate(struct objtool_file *file, static int __annotate_early(struct objtool_file *file, int type, struct in= struction *insn) { switch (type) { + + /* Must be before add_special_section_alts() */ case ANNOTYPE_IGNORE_ALTS: insn->ignore_alts =3D true; break; @@ -3496,11 +3495,6 @@ static int validate_branch(struct objtool_file *file= , struct symbol *func, return 1; } =20 - if (func && insn->ignore) { - WARN_INSN(insn, "BUG: why am I validating an ignored function?"); - return 1; - } - visited =3D VISITED_BRANCH << state.uaccess; if (insn->visited & VISITED_BRANCH_MASK) { if (!insn->hint && !insn_cfi_match(insn, &state.cfi)) @@ -3572,24 +3566,19 @@ static int validate_branch(struct objtool_file *fil= e, struct symbol *func, if (propagate_alt_cfi(file, insn)) return 1; =20 - if (!insn->ignore_alts && insn->alts) { - bool skip_orig =3D false; - + if (insn->alts) { for (alt =3D insn->alts; alt; alt =3D alt->next) { - if (alt->skip_orig) - skip_orig =3D true; - ret =3D validate_branch(file, func, alt->insn, state); if (ret) { BT_INSN(insn, "(alt)"); return ret; } } - - if (skip_orig) - return 0; } =20 + if (insn->alt_group && insn->alt_group->ignore) + return 0; + if (handle_insn_ops(insn, next_insn, &state)) return 1; =20 @@ -3776,23 +3765,15 @@ static int validate_unret(struct objtool_file *file= , struct instruction *insn) =20 insn->visited |=3D VISITED_UNRET; =20 - if (!insn->ignore_alts && insn->alts) { + if (insn->alts) { struct alternative *alt; - bool skip_orig =3D false; - for (alt =3D insn->alts; alt; alt =3D alt->next) { - if (alt->skip_orig) - skip_orig =3D true; - ret =3D validate_unret(file, alt->insn); if (ret) { BT_INSN(insn, "(alt)"); return ret; } } - - if (skip_orig) - return 0; } =20 switch (insn->type) { @@ -3943,7 +3924,7 @@ static bool ignore_unreachable_insn(struct objtool_fi= le *file, struct instructio struct instruction *prev_insn; int i; =20 - if (insn->ignore || insn->type =3D=3D INSN_NOP || insn->type =3D=3D INSN_= TRAP || (func && func->ignore)) + if (insn->type =3D=3D INSN_NOP || insn->type =3D=3D INSN_TRAP || (func &&= func->ignore)) return true; =20 /* diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/= objtool/check.h index e1cd13cd28a3..00fb745e7233 100644 --- a/tools/objtool/include/objtool/check.h +++ b/tools/objtool/include/objtool/check.h @@ -34,6 +34,8 @@ struct alt_group { * This is shared with the other alt_groups in the same alternative. */ struct cfi_state **cfi; + + bool ignore; }; =20 #define INSN_CHUNK_BITS 8 @@ -54,7 +56,6 @@ struct instruction { =20 u32 idx : INSN_CHUNK_BITS, dead_end : 1, - ignore : 1, ignore_alts : 1, hint : 1, save : 1, diff --git a/tools/objtool/include/objtool/special.h b/tools/objtool/includ= e/objtool/special.h index e049679bb17b..72d09c0adf1a 100644 --- a/tools/objtool/include/objtool/special.h +++ b/tools/objtool/include/objtool/special.h @@ -16,8 +16,6 @@ struct special_alt { struct list_head list; =20 bool group; - bool skip_orig; - bool skip_alt; bool jump_or_nop; u8 key_addend; =20 @@ -32,7 +30,7 @@ struct special_alt { =20 int special_get_alts(struct elf *elf, struct list_head *alts); =20 -void arch_handle_alternative(unsigned short feature, struct special_alt *a= lt); +void arch_handle_alternative(struct special_alt *alt); =20 bool arch_support_alt_relocation(struct special_alt *special_alt, struct instruction *insn, diff --git a/tools/objtool/special.c b/tools/objtool/special.c index 097a69db82a0..6cd7b1be5331 100644 --- a/tools/objtool/special.c +++ b/tools/objtool/special.c @@ -54,7 +54,7 @@ static const struct special_entry entries[] =3D { {}, }; =20 -void __weak arch_handle_alternative(unsigned short feature, struct special= _alt *alt) +void __weak arch_handle_alternative(struct special_alt *alt) { } =20 @@ -92,15 +92,7 @@ static int get_alt_entry(struct elf *elf, const struct s= pecial_entry *entry, =20 reloc_to_sec_off(orig_reloc, &alt->orig_sec, &alt->orig_off); =20 - if (entry->feature) { - unsigned short feature; - - feature =3D bswap_if_needed(elf, - *(unsigned short *)(sec->data->d_buf + - offset + - entry->feature)); - arch_handle_alternative(feature, alt); - } + arch_handle_alternative(alt); =20 if (!entry->group || alt->new_len) { new_reloc =3D find_reloc_by_dest(elf, sec, offset + entry->new); --=20 2.48.1