objtool: Fix X86_FEATURE_SMAP alternative handling

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: unreachable instruction

[ mingo: Fix up conflicts with recent x86 changes. ]

Fixes: ea24213d8088 ("objtool: Add UACCESS validation")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/r/de0621ca242130156a55d5d74fed86994dfa4c9c.1742852846.git.jpoimboe@kernel.org
Closes: https://lore.kernel.org/oe-kbuild-all/202503181736.zkZUBv4N-lkp@intel.com/

diff --git a/arch/x86/include/asm/arch_hweight.h b/arch/x86/include/asm/arch_hweight.h
index b5982b94bdbaf9833687ef3f5993b020c1e12bd7..cbc6157f0b4b7a817ab05e98c896f2c84fbfc2c1 100644 (file)
--- a/arch/x86/include/asm/arch_hweight.h
+++ b/arch/x86/include/asm/arch_hweight.h
@@ -16,7 +16,8 @@ static __always_inline unsigned int __arch_hweight32(unsigned int w)
 {
        unsigned int res;
 
-       asm_inline (ALTERNATIVE("call __sw_hweight32",
+       asm_inline (ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE
+                               "call __sw_hweight32",
                                "popcntl %[val], %[cnt]", X86_FEATURE_POPCNT)
                         : [cnt] "=" REG_OUT (res), ASM_CALL_CONSTRAINT
                         : [val] REG_IN (w));
@@ -45,7 +46,8 @@ static __always_inline unsigned long __arch_hweight64(__u64 w)
 {
        unsigned long res;
 
-       asm_inline (ALTERNATIVE("call __sw_hweight64",
+       asm_inline (ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE
+                               "call __sw_hweight64",
                                "popcntq %[val], %[cnt]", X86_FEATURE_POPCNT)
                         : [cnt] "=" REG_OUT (res), ASM_CALL_CONSTRAINT
                         : [val] REG_IN (w));

diff --git a/arch/x86/include/asm/smap.h b/arch/x86/include/asm/smap.h
index daea94c2993c5faaabd0f28c9bdf5b87516e009d..55a5e656e4b9ea861825a35a6f6fbc14817511ef 100644 (file)
--- a/arch/x86/include/asm/smap.h
+++ b/arch/x86/include/asm/smap.h
@@ -16,23 +16,23 @@
 #ifdef __ASSEMBLER__
 
 #define ASM_CLAC \
-       ALTERNATIVE "", "clac", X86_FEATURE_SMAP
+       ALTERNATIVE __stringify(ANNOTATE_IGNORE_ALTERNATIVE), "clac", X86_FEATURE_SMAP
 
 #define ASM_STAC \
-       ALTERNATIVE "", "stac", X86_FEATURE_SMAP
+       ALTERNATIVE __stringify(ANNOTATE_IGNORE_ALTERNATIVE), "stac", X86_FEATURE_SMAP
 
 #else /* __ASSEMBLER__ */
 
 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);
 }
 
 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);
 }
 
 static __always_inline unsigned long smap_save(void)
@@ -40,7 +40,8 @@ static __always_inline unsigned long smap_save(void)
        unsigned long flags;
 
        asm volatile ("# smap_save\n\t"
-                     ALTERNATIVE("", "pushf; pop %0; " "clac" "\n\t",
+                     ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE
+                                 "", "pushf; pop %0; clac",
                                  X86_FEATURE_SMAP)
                      : "=rm" (flags) : : "memory", "cc");
 
@@ -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");
 }
 
 /* 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)
 
 #endif /* __ASSEMBLER__ */
 

diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index 97771b9d33af30fbaf22ac22fd6f4281b8031fa6..59a62c3780a2f2c6884393936f261ccd22b9c9d5 100644 (file)
--- 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=1.
         */
-       asm volatile(ANNOTATE_IGNORE_ALTERNATIVE
-                    ASM_STAC ::: "memory", "flags");
+       asm volatile(ASM_STAC_UNSAFE ::: "memory", "flags");
 }
 
 static __always_inline void __xen_clac(void)
 {
-       asm volatile(ANNOTATE_IGNORE_ALTERNATIVE
-                    ASM_CLAC ::: "memory", "flags");
+       asm volatile(ASM_CLAC_UNSAFE ::: "memory", "flags");
 }
 
 static inline long

diff --git a/tools/objtool/arch/x86/special.c b/tools/objtool/arch/x86/special.c
index 5f46d4e7f7f8af62b8038a2b6c64e2e1fed60eae..403e587676f1b1829e257d945663bd40594d77d4 100644 (file)
--- a/tools/objtool/arch/x86/special.c
+++ b/tools/objtool/arch/x86/special.c
@@ -5,10 +5,7 @@
 #include <objtool/builtin.h>
 #include <objtool/warn.h>
 
-#define X86_FEATURE_POPCNT (4 * 32 + 23)
-#define X86_FEATURE_SMAP   (9 * 32 + 20)
-
-void arch_handle_alternative(unsigned short feature, struct special_alt *alt)
+void arch_handle_alternative(struct special_alt *alt)
 {
        static struct special_alt *group, *prev;
 
@@ -32,34 +29,6 @@ void arch_handle_alternative(unsigned short feature, struct special_alt *alt)
        } else group = alt;
 
        prev = 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 = true;
-               else
-                       alt->skip_alt = 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 = true;
-               break;
-       default:
-               break;
-       }
 }
 
 bool arch_support_alt_relocation(struct special_alt *special_alt,

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f4650205854d30a58f3c5216c147b42b96ec08e2..b2f6a7ff77b50cb86d723efb938fed18d709031e 100644 (file)
--- 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;
 };
 
 static unsigned long nr_cfi, nr_cfi_reused, nr_cfi_cache;
@@ -1696,6 +1695,7 @@ static int handle_group_alt(struct objtool_file *file,
                orig_alt_group->first_insn = orig_insn;
                orig_alt_group->last_insn = last_orig_insn;
                orig_alt_group->nop = NULL;
+               orig_alt_group->ignore = orig_insn->ignore_alts;
        } else {
                if (orig_alt_group->last_insn->offset + orig_alt_group->last_insn->len -
                    orig_alt_group->first_insn->offset != special_alt->orig_len) {
@@ -1735,7 +1735,6 @@ static int handle_group_alt(struct objtool_file *file,
                nop->type = INSN_NOP;
                nop->sym = orig_insn->sym;
                nop->alt_group = new_alt_group;
-               nop->ignore = orig_insn->ignore_alts;
        }
 
        if (!special_alt->new_len) {
@@ -1752,7 +1751,6 @@ static int handle_group_alt(struct objtool_file *file,
 
                last_new_insn = insn;
 
-               insn->ignore = orig_insn->ignore_alts;
                insn->sym = orig_insn->sym;
                insn->alt_group = new_alt_group;
 
@@ -1799,6 +1797,7 @@ end:
        new_alt_group->first_insn = *new_insn;
        new_alt_group->last_insn = last_new_insn;
        new_alt_group->nop = nop;
+       new_alt_group->ignore = (*new_insn)->ignore_alts;
        new_alt_group->cfi = orig_alt_group->cfi;
        return 0;
 }
@@ -1916,8 +1915,6 @@ static int add_special_section_alts(struct objtool_file *file)
                }
 
                alt->insn = new_insn;
-               alt->skip_orig = special_alt->skip_orig;
-               orig_insn->ignore_alts |= special_alt->skip_alt;
                alt->next = orig_insn->alts;
                orig_insn->alts = alt;
 
@@ -2295,6 +2292,8 @@ static int read_annotate(struct objtool_file *file,
 static int __annotate_early(struct objtool_file *file, int type, struct instruction *insn)
 {
        switch (type) {
+
+       /* Must be before add_special_section_alts() */
        case ANNOTYPE_IGNORE_ALTS:
                insn->ignore_alts = true;
                break;
@@ -3488,11 +3487,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
                        return 1;
                }
 
-               if (func && insn->ignore) {
-                       WARN_INSN(insn, "BUG: why am I validating an ignored function?");
-                       return 1;
-               }
-
                visited = VISITED_BRANCH << state.uaccess;
                if (insn->visited & VISITED_BRANCH_MASK) {
                        if (!insn->hint && !insn_cfi_match(insn, &state.cfi))
@@ -3564,24 +3558,19 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
                if (propagate_alt_cfi(file, insn))
                        return 1;
 
-               if (!insn->ignore_alts && insn->alts) {
-                       bool skip_orig = false;
-
+               if (insn->alts) {
                        for (alt = insn->alts; alt; alt = alt->next) {
-                               if (alt->skip_orig)
-                                       skip_orig = true;
-
                                ret = validate_branch(file, func, alt->insn, state);
                                if (ret) {
                                        BT_INSN(insn, "(alt)");
                                        return ret;
                                }
                        }
-
-                       if (skip_orig)
-                               return 0;
                }
 
+               if (insn->alt_group && insn->alt_group->ignore)
+                       return 0;
+
                if (handle_insn_ops(insn, next_insn, &state))
                        return 1;
 
@@ -3768,23 +3757,15 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
 
                insn->visited |= VISITED_UNRET;
 
-               if (!insn->ignore_alts && insn->alts) {
+               if (insn->alts) {
                        struct alternative *alt;
-                       bool skip_orig = false;
-
                        for (alt = insn->alts; alt; alt = alt->next) {
-                               if (alt->skip_orig)
-                                       skip_orig = true;
-
                                ret = validate_unret(file, alt->insn);
                                if (ret) {
                                        BT_INSN(insn, "(alt)");
                                        return ret;
                                }
                        }
-
-                       if (skip_orig)
-                               return 0;
                }
 
                switch (insn->type) {
@@ -3935,7 +3916,7 @@ static bool ignore_unreachable_insn(struct objtool_file *file, struct instructio
        struct instruction *prev_insn;
        int i;
 
-       if (insn->ignore || insn->type == INSN_NOP || insn->type == INSN_TRAP || (func && func->ignore))
+       if (insn->type == INSN_NOP || insn->type == INSN_TRAP || (func && func->ignore))
                return true;
 
        /*

diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h
index e1cd13cd28a377840a48d2d6eb3bca8a356c0463..00fb745e723391839e822db4c4c0706b34a26b55 100644 (file)
--- 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;
 };
 
 #define INSN_CHUNK_BITS                8
@@ -54,7 +56,6 @@ struct instruction {
 
        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/include/objtool/special.h
index e049679bb17b2668f53e27646acad43ec79c40f7..72d09c0adf1a187a4716934c83c0dad939ec1c10 100644 (file)
--- 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;
 
        bool group;
-       bool skip_orig;
-       bool skip_alt;
        bool jump_or_nop;
        u8 key_addend;
 
@@ -32,7 +30,7 @@ struct special_alt {
 
 int special_get_alts(struct elf *elf, struct list_head *alts);
 
-void arch_handle_alternative(unsigned short feature, struct special_alt *alt);
+void arch_handle_alternative(struct special_alt *alt);
 
 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 097a69db82a0ee64ce67cf7a6b605126b1e56a14..6cd7b1be53313ddb8f8ed78d443a18f4ee8a56f9 100644 (file)
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -54,7 +54,7 @@ static const struct special_entry entries[] = {
        {},
 };
 
-void __weak arch_handle_alternative(unsigned short feature, struct special_alt *alt)
+void __weak arch_handle_alternative(struct special_alt *alt)
 {
 }
 
@@ -92,15 +92,7 @@ static int get_alt_entry(struct elf *elf, const struct special_entry *entry,
 
        reloc_to_sec_off(orig_reloc, &alt->orig_sec, &alt->orig_off);
 
-       if (entry->feature) {
-               unsigned short feature;
-
-               feature = bswap_if_needed(elf,
-                                         *(unsigned short *)(sec->data->d_buf +
-                                                             offset +
-                                                             entry->feature));
-               arch_handle_alternative(feature, alt);
-       }
+       arch_handle_alternative(alt);
 
        if (!entry->group || alt->new_len) {
                new_reloc = find_reloc_by_dest(elf, sec, offset + entry->new);