[RFC] objtool: STAC/CLAC validation

Commit Message

Peter Zijlstra Feb. 22, 2019, 10:26 p.m. UTC

On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:

> But correct :)

> I agree, that a function which is doing the actual copy should be callable,
> but random other functions? NO!

So find the below patch -- which spotted fail in ptrace.c

It has an AC_SAFE(func) annotation which allows marking specific
functions as safe to call. The patch includes 2 instances which were
required to make arch/x86 'build':

  arch/x86/ia32/ia32_signal.o: warning: objtool: ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC set
  arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to getreg() with AC set

It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the
lack of notrace annotations on functions marked AC_SAFE():

  arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to __fentry__() with AC set

It builds arch/x86 relatively clean; it only complains about some
redundant CLACs in entry_64.S because it doesn't understand interrupts
and I've not bothered creating an annotation for them yet.

  arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x4d: redundant CLAC
  arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x5a: redundant CLAC
  ...
  arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC

Also, I realized we don't need special annotations for preempt_count;
preempt_disable() emits a CALL instruction which should readily trigger
the warnings added here.

The VDSO thing is a bit of a hack, but I couldn't quickly find anything
better.

Comments?

---
 arch/x86/include/asm/special_insns.h |  2 ++
 arch/x86/kernel/ptrace.c             |  3 +-
 include/linux/frame.h                | 23 ++++++++++++++
 tools/objtool/arch.h                 |  4 ++-
 tools/objtool/arch/x86/decode.c      | 14 ++++++++-
 tools/objtool/check.c                | 59 ++++++++++++++++++++++++++++++++++++
 tools/objtool/check.h                |  3 +-
 tools/objtool/elf.h                  |  1 +
 8 files changed, 105 insertions(+), 4 deletions(-)

Comments

Linus Torvalds Feb. 22, 2019, 11:34 p.m. UTC | #1

On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <[e-mail]> wrote:
>
> So find the below patch -- which spotted fail in ptrace.c
>
> It has an AC_SAFE(func) annotation which allows marking specific
> functions as safe to call. The patch includes 2 instances which were
> required to make arch/x86 'build':

Looks sane enough to me.

Can you make it do DF too while at it? I really think AC and DF are
the same in this context. If you call an arbitrary function with DF
set, things will very quickly go sideways (even if it might work in
practice as long as the function just doesn't do a memcpy or similar)

              Linus

H. Peter Anvin Feb. 22, 2019, 11:39 p.m. UTC | #2

On February 22, 2019 2:26:35 PM PST, Peter Zijlstra <[e-mail]> wrote:
>On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
>
>> But correct :)
>
>> I agree, that a function which is doing the actual copy should be
>callable,
>> but random other functions? NO!
>
>So find the below patch -- which spotted fail in ptrace.c
>
>It has an AC_SAFE(func) annotation which allows marking specific
>functions as safe to call. The patch includes 2 instances which were
>required to make arch/x86 'build':
>
>arch/x86/ia32/ia32_signal.o: warning: objtool:
>ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC
>set
>arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to
>getreg() with AC set
>
>It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the
>lack of notrace annotations on functions marked AC_SAFE():
>
>arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to
>__fentry__() with AC set
>
>It builds arch/x86 relatively clean; it only complains about some
>redundant CLACs in entry_64.S because it doesn't understand interrupts
>and I've not bothered creating an annotation for them yet.
>
>arch/x86/entry/entry_64.o: warning: objtool:
>.altinstr_replacement+0x4d: redundant CLAC
>arch/x86/entry/entry_64.o: warning: objtool:
>.altinstr_replacement+0x5a: redundant CLAC
>  ...
>arch/x86/entry/entry_64.o: warning: objtool:
>.altinstr_replacement+0xb1: redundant CLAC
>
>Also, I realized we don't need special annotations for preempt_count;
>preempt_disable() emits a CALL instruction which should readily trigger
>the warnings added here.
>
>The VDSO thing is a bit of a hack, but I couldn't quickly find anything
>better.
>
>Comments?
>
>---
> arch/x86/include/asm/special_insns.h |  2 ++
> arch/x86/kernel/ptrace.c             |  3 +-
> include/linux/frame.h                | 23 ++++++++++++++
> tools/objtool/arch.h                 |  4 ++-
> tools/objtool/arch/x86/decode.c      | 14 ++++++++-
>tools/objtool/check.c                | 59
>++++++++++++++++++++++++++++++++++++
> tools/objtool/check.h                |  3 +-
> tools/objtool/elf.h                  |  1 +
> 8 files changed, 105 insertions(+), 4 deletions(-)
>
>diff --git a/arch/x86/include/asm/special_insns.h
>b/arch/x86/include/asm/special_insns.h
>index 43c029cdc3fe..cd31e4433f4c 100644
>--- a/arch/x86/include/asm/special_insns.h
>+++ b/arch/x86/include/asm/special_insns.h
>@@ -5,6 +5,7 @@
> 
> #ifdef __KERNEL__
> 
>+#include <linux/frame.h>
> #include <asm/nops.h>
> 
> /*
>@@ -135,6 +136,7 @@ static inline void native_wbinvd(void)
> }
> 
> extern asmlinkage void native_load_gs_index(unsigned);
>+AC_SAFE(native_load_gs_index);
> 
> static inline unsigned long __read_cr4(void)
> {
>diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
>index 4b8ee05dd6ad..e278b4055a8b 100644
>--- a/arch/x86/kernel/ptrace.c
>+++ b/arch/x86/kernel/ptrace.c
>@@ -420,7 +420,7 @@ static int putreg(struct task_struct *child,
> 	return 0;
> }
> 
>-static unsigned long getreg(struct task_struct *task, unsigned long
>offset)
>+static notrace unsigned long getreg(struct task_struct *task, unsigned
>long offset)
> {
> 	switch (offset) {
> 	case offsetof(struct user_regs_struct, cs):
>@@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct
>*task, unsigned long offset)
> 
> 	return *pt_regs_access(task_pt_regs(task), offset);
> }
>+AC_SAFE(getreg);
> 
> static int genregs_get(struct task_struct *target,
> 		       const struct user_regset *regset,
>diff --git a/include/linux/frame.h b/include/linux/frame.h
>index 02d3ca2d9598..5d354cf42a56 100644
>--- a/include/linux/frame.h
>+++ b/include/linux/frame.h
>@@ -21,4 +21,27 @@
> 
> #endif /* CONFIG_STACK_VALIDATION */
> 
>+#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) &&
>!defined(BUILD_VDSO32)
>+/*
>+ * This macro marks functions as AC-safe, that is, it is safe to call
>from an
>+ * EFLAGS.AC enabled region (typically user_access_begin() /
>+ * user_access_end()).
>+ *
>+ * These functions in turn will only call AC-safe functions themselves
>(which
>+ * precludes tracing, including __fentry__ and scheduling, including
>+ * preempt_enable).
>+ *
>+ * AC-safe functions will obviously also not change EFLAGS.AC
>themselves.
>+ *
>+ * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO
>builds
>+ * (and the generated symbol reference will in fact cause link
>failures).
>+ */
>+#define AC_SAFE(func) \
>+	static void __used __section(.discard.ac_safe) \
>+		*__func_ac_safe_##func = func
>+
>+#else
>+#define AC_SAFE(func)
>+#endif
>+
> #endif /* _LINUX_FRAME_H */
>diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
>index b0d7dc3d71b5..48327099466d 100644
>--- a/tools/objtool/arch.h
>+++ b/tools/objtool/arch.h
>@@ -33,7 +33,9 @@
> #define INSN_STACK		8
> #define INSN_BUG		9
> #define INSN_NOP		10
>-#define INSN_OTHER		11
>+#define INSN_STAC		11
>+#define INSN_CLAC		12
>+#define INSN_OTHER		13
> #define INSN_LAST		INSN_OTHER
> 
> enum op_dest_type {
>diff --git a/tools/objtool/arch/x86/decode.c
>b/tools/objtool/arch/x86/decode.c
>index 540a209b78ab..d1e99d1460a5 100644
>--- a/tools/objtool/arch/x86/decode.c
>+++ b/tools/objtool/arch/x86/decode.c
>@@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *elf,
>struct section *sec,
> 
> 	case 0x0f:
> 
>-		if (op2 >= 0x80 && op2 <= 0x8f) {
>+		if (op2 == 0x01) {
>+
>+			if (modrm == 0xca) {
>+
>+				*type = INSN_CLAC;
>+
>+			} else if (modrm == 0xcb) {
>+
>+				*type = INSN_STAC;
>+
>+			}
>+
>+		} else if (op2 >= 0x80 && op2 <= 0x8f) {
> 
> 			*type = INSN_JUMP_CONDITIONAL;
> 
>diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>index 0414a0d52262..01852602ca31 100644
>--- a/tools/objtool/check.c
>+++ b/tools/objtool/check.c
>@@ -127,6 +127,24 @@ static bool ignore_func(struct objtool_file *file,
>struct symbol *func)
> 	return false;
> }
> 
>+static bool ac_safe_func(struct objtool_file *file, struct symbol
>*func)
>+{
>+	struct rela *rela;
>+
>+	/* check for AC_SAFE */
>+	if (file->ac_safe && file->ac_safe->rela)
>+		list_for_each_entry(rela, &file->ac_safe->rela->rela_list, list) {
>+			if (rela->sym->type == STT_SECTION &&
>+			    rela->sym->sec == func->sec &&
>+			    rela->addend == func->offset)
>+				return true;
>+			if (/* rela->sym->type == STT_FUNC && */ rela->sym == func)
>+				return true;
>+		}
>+
>+	return false;
>+}
>+
> /*
>  * This checks to see if the given function is a "noreturn" function.
>  *
>@@ -439,6 +457,8 @@ static void add_ignores(struct objtool_file *file)
> 
> 	for_each_sec(file, sec) {
> 		list_for_each_entry(func, &sec->symbol_list, list) {
>+			func->ac_safe = ac_safe_func(file, func);
>+
> 			if (func->type != STT_FUNC)
> 				continue;
> 
>@@ -1902,6 +1922,11 @@ static int validate_branch(struct objtool_file
>*file, struct instruction *first,
> 		switch (insn->type) {
> 
> 		case INSN_RETURN:
>+			if (state.ac) {
>+				WARN_FUNC("return with AC set", sec, insn->offset);
>+				return 1;
>+			}
>+
> 			if (func && has_modified_stack_frame(&state)) {
> 				WARN_FUNC("return with modified stack frame",
> 					  sec, insn->offset);
>@@ -1917,6 +1942,12 @@ static int validate_branch(struct objtool_file
>*file, struct instruction *first,
> 			return 0;
> 
> 		case INSN_CALL:
>+			if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
>+				WARN_FUNC("call to %s() with AC set", sec, insn->offset,
>+						insn->call_dest->name);
>+				return 1;
>+			}
>+
> 			if (is_fentry_call(insn))
> 				break;
> 
>@@ -1928,6 +1959,11 @@ static int validate_branch(struct objtool_file
>*file, struct instruction *first,
> 
> 			/* fallthrough */
> 		case INSN_CALL_DYNAMIC:
>+			if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
>+				WARN_FUNC("call to %s() with AC set", sec, insn->offset,
>+						insn->call_dest->name);
>+				return 1;
>+			}
> 			if (!no_fp && func && !has_valid_stack_frame(&state)) {
> 				WARN_FUNC("call without frame pointer save/setup",
> 					  sec, insn->offset);
>@@ -1980,6 +2016,26 @@ static int validate_branch(struct objtool_file
>*file, struct instruction *first,
> 
> 			break;
> 
>+		case INSN_STAC:
>+			if (state.ac_safe || state.ac) {
>+				WARN_FUNC("recursive STAC", sec, insn->offset);
>+				return 1;
>+			}
>+			state.ac = true;
>+			break;
>+
>+		case INSN_CLAC:
>+			if (!state.ac) {
>+				WARN_FUNC("redundant CLAC", sec, insn->offset);
>+				return 1;
>+			}
>+			if (state.ac_safe) {
>+				WARN_FUNC("AC-safe clears AC", sec, insn->offset);
>+				return 1;
>+			}
>+			state.ac = false;
>+			break;
>+
> 		default:
> 			break;
> 		}
>@@ -2141,6 +2197,8 @@ static int validate_functions(struct objtool_file
>*file)
> 			if (!insn || insn->ignore)
> 				continue;
> 
>+			state.ac_safe = func->ac_safe;
>+
> 			ret = validate_branch(file, insn, state);
> 			warnings += ret;
> 		}
>@@ -2198,6 +2256,7 @@ int check(const char *_objname, bool orc)
> 	INIT_LIST_HEAD(&file.insn_list);
> 	hash_init(file.insn_hash);
>	file.whitelist = find_section_by_name(file.elf,
>".discard.func_stack_frame_non_standard");
>+	file.ac_safe = find_section_by_name(file.elf, ".discard.ac_safe");
> 	file.c_file = find_section_by_name(file.elf, ".comment");
> 	file.ignore_unreachables = no_unreachable;
> 	file.hints = false;
>diff --git a/tools/objtool/check.h b/tools/objtool/check.h
>index e6e8a655b556..c31ec3ca78f3 100644
>--- a/tools/objtool/check.h
>+++ b/tools/objtool/check.h
>@@ -31,7 +31,7 @@ struct insn_state {
> 	int stack_size;
> 	unsigned char type;
> 	bool bp_scratch;
>-	bool drap, end;
>+	bool drap, end, ac, ac_safe;
> 	int drap_reg, drap_offset;
> 	struct cfi_reg vals[CFI_NUM_REGS];
> };
>@@ -61,6 +61,7 @@ struct objtool_file {
> 	struct list_head insn_list;
> 	DECLARE_HASHTABLE(insn_hash, 16);
> 	struct section *whitelist;
>+	struct section *ac_safe;
> 	bool ignore_unreachables, c_file, hints, rodata;
> };
> 
>diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
>index bc97ed86b9cd..064c3df31e40 100644
>--- a/tools/objtool/elf.h
>+++ b/tools/objtool/elf.h
>@@ -62,6 +62,7 @@ struct symbol {
> 	unsigned long offset;
> 	unsigned int len;
> 	struct symbol *pfunc, *cfunc;
>+	bool ac_safe;
> };
> 
> struct rela {

I like it.

Objtool could also detect CLAC-STAC or STAC-CLAC sequences without memory operations and remove them; don't know how often that happens, but I know it *does* happen.

Andy Lutomirski Feb. 22, 2019, 11:55 p.m. UTC | #3

On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <[e-mail]> wrote:
>
> On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
>
> > But correct :)
>
> > I agree, that a function which is doing the actual copy should be callable,
> > but random other functions? NO!
>
> So find the below patch -- which spotted fail in ptrace.c
>
> It has an AC_SAFE(func) annotation which allows marking specific
> functions as safe to call. The patch includes 2 instances which were
> required to make arch/x86 'build':
>
>   arch/x86/ia32/ia32_signal.o: warning: objtool: ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC set
>   arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to getreg() with AC set
>
> It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the
> lack of notrace annotations on functions marked AC_SAFE():
>
>   arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to __fentry__() with AC set
>
> It builds arch/x86 relatively clean; it only complains about some
> redundant CLACs in entry_64.S because it doesn't understand interrupts
> and I've not bothered creating an annotation for them yet.
>
>   arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x4d: redundant CLAC
>   arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x5a: redundant CLAC
>   ...
>   arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC

Does objtool understand altinstr?  If it understands that this is an
altinstr associated with a not-actually-a-fuction (i.e. END not
ENDPROC) piece of code, it could suppress this warning.

>
> -static unsigned long getreg(struct task_struct *task, unsigned long offset)
> +static notrace unsigned long getreg(struct task_struct *task, unsigned long offset)
>  {
>         switch (offset) {
>         case offsetof(struct user_regs_struct, cs):
> @@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task, unsigned long offset)
>
>         return *pt_regs_access(task_pt_regs(task), offset);
>  }
> +AC_SAFE(getreg);

This takes the address and could plausibly suppress some optimizations.

>
> +#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) && !defined(BUILD_VDSO32)
> +/*
> + * This macro marks functions as AC-safe, that is, it is safe to call from an
> + * EFLAGS.AC enabled region (typically user_access_begin() /
> + * user_access_end()).
> + *
> + * These functions in turn will only call AC-safe functions themselves (which
> + * precludes tracing, including __fentry__ and scheduling, including
> + * preempt_enable).
> + *
> + * AC-safe functions will obviously also not change EFLAGS.AC themselves.
> + *
> + * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO builds
> + * (and the generated symbol reference will in fact cause link failures).
> + */
> +#define AC_SAFE(func) \
> +       static void __used __section(.discard.ac_safe) \
> +               *__func_ac_safe_##func = func

Are we okay with annotating function *declarations* in a way that
their addresses get taken whenever the declaration is processed?  It
would be nice if we could avoid this.

I'm wondering if we can just change the code that does getreg() and
load_gs_index() so it doesn't do it with AC set.  Also, what about
paravirt kernels?  They'll call into PV code for load_gs_index() with
AC set.

--Andy

Andy Lutomirski Feb. 23, 2019, 12:34 a.m. UTC | #4

[mailing lists removed because this is a potentially large source of exploits]

On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <[e-mail]> wrote:
>
> On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
>
> > But correct :)
>
> > I agree, that a function which is doing the actual copy should be callable,
> > but random other functions? NO!
>
> So find the below patch -- which spotted fail in ptrace.c
>

Um, wait a moment.  You didn't find an oddity in ptrace.c.  You found
a giant freaking error in uaccess.h.

Am I missing something?  How are there not zillions of instances of
this that your patch ought to catch?  Or is genregs_get() really the
only example?
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 780f2b42c8ef..df0571a07b55 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -431,8 +431,10 @@ do {									\
 ({								\
 	__label__ __pu_label;					\
 	int __pu_err = -EFAULT;					\
+	__typeof__(*(ptr)) __pu_val;				\
+	__pu_val = x;						\
 	__uaccess_begin();					\
-	__put_user_size((x), (ptr), (size), __pu_label);	\
+	__put_user_size(__pu_val, (ptr), (size), __pu_label);	\
 	__pu_err = 0;						\
 __pu_label:							\
 	__uaccess_end();					\

Linus Torvalds Feb. 23, 2019, 1:12 a.m. UTC | #5

On Fri, Feb 22, 2019 at 4:34 PM Andy Lutomirski <[e-mail]> wrote:
>
> [mailing lists removed because this is a potentially large source of exploits]

I think you're overly worried.

AC doesn't protect against "large source of exploits". If it did, then
all CPU's before Broadwell would be insecure. They aren't. They'd
better not be, considering that there's a _lot_ of Xeon machines out
there based on older microarchitectures. I think some of them might
even be reasonably current (eg Xeon E7v3 isn't _that_ old, and is
Haswell-based, and doesn't have SMAP afaik).

SMAP is a great debugging and development aid, and makes sure that
developers - who hopefully run primarily on modern platforms - don't
write code that just accesses user space directly (because with SMAP,
it won't work).

And yes, SMAP can limit the effect of kernel bugs, and turn something
that would otherwise be a security issue into "just a bug".

But running with AC on isn't a security issue in itself - it just
makes SMAP slightly less powerful. The biggest issue of the whole "AC
doesn't get saved/redstored" is actually the *reverse* case, where a
preemption event could then cause a process that had AC on to be
scheduled away, then AC would stay on for some time, but then we might
schedule back with AC _clear_, and now you'd have a non-working user
access, and a possible DoS attack as a result because you returned
EFAULT to a system call that was perfectly fine.

See? It's not so much "AC stays on" that is a "sky is falling" issue,
it's actually "AC also gets turned off randomly" that actually has
real and immediate effects. "AC on" is unfortunate and not great,
don't get me wrong, but it's definitely not the end of the world.
Particularly not for short sequences.

That said:

> Um, wait a moment.  You didn't find an oddity in ptrace.c.  You found
> a giant freaking error in uaccess.h.

I agree that your patch is good, and should be applied. Mind writing
up a changelog and committing it to -tip?

> Am I missing something?  How are there not zillions of instances of
> this that your patch ought to catch?  Or is genregs_get() really the
> only example?

I really do think that it's very unusual to do "get/put_user()" with
complicated value arguments. So while your patch is obviously the
right thing to do, I really don't think this is a huge worry, or all
_that_ surprising that this issue apparently found just a single case
of a function call.

With all that said, I didn't even react to this part of PeterZ's
patch, but it's a good call, and I think it's also a great validation
of the objtool approach to validating AC. So cheers for that!

                Linus

Andy Lutomirski Feb. 23, 2019, 1:16 a.m. UTC | #6

On Fri, Feb 22, 2019 at 5:12 PM Linus Torvalds
<[e-mail]> wrote:
>
> On Fri, Feb 22, 2019 at 4:34 PM Andy Lutomirski <[e-mail]> wrote:
> >
> > [mailing lists removed because this is a potentially large source of exploits]
>
> I think you're overly worried.
>
> AC doesn't protect against "large source of exploits".

What I meant was: a potentially large grab bag of ways to help turn a
bug into an exploit.

> That said:
>
> > Um, wait a moment.  You didn't find an oddity in ptrace.c.  You found
> > a giant freaking error in uaccess.h.
>
> I agree that your patch is good, and should be applied. Mind writing
> up a changelog and committing it to -tip?

I don't have commit access :)  But I shall email it out once I test it a bit.

--Andy

Linus Torvalds Feb. 23, 2019, 1:33 a.m. UTC | #7

On Fri, Feb 22, 2019 at 5:17 PM Andy Lutomirski <[e-mail]> wrote:
>
> What I meant was: a potentially large grab bag of ways to help turn a
> bug into an exploit.

Well, apparently there's only one place, and that one looks pretty harmless.

Of course, I don't think PeterZ said what his config was. Maybe it was
a very minimal config and only found that one case because all the
truly scary cases were configured out ;)

                   Linus

Linus Torvalds Feb. 23, 2019, 1:40 a.m. UTC | #8

On Fri, Feb 22, 2019 at 5:17 PM Andy Lutomirski <[e-mail]> wrote:
>
> I don't have commit access :)  But I shall email it out once I test it a bit.

Btw, looking at it, with that change every user of __put_user_size()
now passes in the properly type-cast value in 'x'.

Which means that you could remove the odd typecast in the 8-byte case:

        case 8:                                                         \
                __put_user_goto_u64((__typeof__(*ptr))(x), ptr, label); \

and make it match all the other cases:

        case 8:                                                         \
                __put_user_goto_u64(x, ptr, label);                     \

but that's just from looking at the patch, and maybe I'm missing something.

                  Linus

Peter Zijlstra Feb. 23, 2019, 8:37 a.m. UTC | #9

On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote:
> On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <[e-mail]> wrote:

> >   arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC
> 
> Does objtool understand altinstr?

Yep, otherwise it would've never found any of the STAC/CLAC instructions
to begin with, they're all alternatives. The emitted code is all NOPs.

> If it understands that this is an
> altinstr associated with a not-actually-a-fuction (i.e. END not
> ENDPROC) piece of code, it could suppress this warning.

Using readelf on entry_64.o the symbol type appears to be STT_NOTYPE for
these 'functions', so yes, I can try and ignore this warning for those.

> > +#define AC_SAFE(func) \
> > +       static void __used __section(.discard.ac_safe) \
> > +               *__func_ac_safe_##func = func
> 
> Are we okay with annotating function *declarations* in a way that
> their addresses get taken whenever the declaration is processed?  It
> would be nice if we could avoid this.
> 
> I'm wondering if we can just change the code that does getreg() and
> load_gs_index() so it doesn't do it with AC set.  Also, what about
> paravirt kernels?  They'll call into PV code for load_gs_index() with
> AC set.

Fixing that code would be fine; but we need that annotation regardless,
read Linus' email a little further back, he wants things like
copy_{to,from}_user_unsafe().

Those really would need to be marked AC-safe, there's no inlining that.

That said, yes these annotations _suck_. But it's what we had and works,
I'm just copying it (from STACK_FRAME_NON_STANDARD).

That said; maybe we can do something like:

#define AC_SAFE(func)						\
	asm (".pushsection .discard.ac_safe_sym\n\t"		\
	     "999: .ascii \"" #func "\"\n\t"			\
	     ".popsection\n\t"					\
	     ".pushsection .discard.ac_safe\n\t"		\
	     _ASM_PTR " 999b\n\t"				\
	     ".popsection")

I just don't have a clue on how to read that in objtool; weak elf foo.
I'll see if I can make it work.

Peter Zijlstra Feb. 23, 2019, 8:39 a.m. UTC | #10

On Fri, Feb 22, 2019 at 03:39:48PM -0800, [e-mail] wrote:
> Objtool could also detect CLAC-STAC or STAC-CLAC sequences without
> memory operations and remove them; don't know how often that happens,
> but I know it *does* happen.

Objtool doesn't know about memops; that'd be a lot of work. Also,
objtool doesn't actually rewrite the text, at best it could warn about
such occurences.

Peter Zijlstra Feb. 23, 2019, 8:43 a.m. UTC | #11

On Fri, Feb 22, 2019 at 03:34:00PM -0800, Linus Torvalds wrote:
> Can you make it do DF too while at it?

Sure.

Peter Zijlstra Feb. 23, 2019, 10:52 a.m. UTC | #12

On Sat, Feb 23, 2019 at 09:37:06AM +0100, Peter Zijlstra wrote:
> On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote:
> > On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <[e-mail]> wrote:
> 
> > >   arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC
> > 
> > Does objtool understand altinstr?
> 
> Yep, otherwise it would've never found any of the STAC/CLAC instructions
> to begin with, they're all alternatives. The emitted code is all NOPs.
> 
> > If it understands that this is an
> > altinstr associated with a not-actually-a-fuction (i.e. END not
> > ENDPROC) piece of code, it could suppress this warning.
> 
> Using readelf on entry_64.o the symbol type appears to be STT_NOTYPE for
> these 'functions', so yes, I can try and ignore this warning for those.
> 
> > > +#define AC_SAFE(func) \
> > > +       static void __used __section(.discard.ac_safe) \
> > > +               *__func_ac_safe_##func = func
> > 
> > Are we okay with annotating function *declarations* in a way that
> > their addresses get taken whenever the declaration is processed?  It
> > would be nice if we could avoid this.
> > 
> > I'm wondering if we can just change the code that does getreg() and
> > load_gs_index() so it doesn't do it with AC set.  Also, what about
> > paravirt kernels?  They'll call into PV code for load_gs_index() with
> > AC set.
> 
> Fixing that code would be fine; but we need that annotation regardless,
> read Linus' email a little further back, he wants things like
> copy_{to,from}_user_unsafe().
> 
> Those really would need to be marked AC-safe, there's no inlining that.
> 
> That said, yes these annotations _suck_. But it's what we had and works,
> I'm just copying it (from STACK_FRAME_NON_STANDARD).
> 
> That said; maybe we can do something like:
> 
> #define AC_SAFE(func)						\
> 	asm (".pushsection .discard.ac_safe_sym\n\t"		\
> 	     "999: .ascii \"" #func "\"\n\t"			\
> 	     ".popsection\n\t"					\
> 	     ".pushsection .discard.ac_safe\n\t"		\
> 	     _ASM_PTR " 999b\n\t"				\
> 	     ".popsection")
> 
> I just don't have a clue on how to read that in objtool; weak elf foo.
> I'll see if I can make it work.

Fixed all that. Should probably also convert that NON_STANDARD stuff to
this new shiny thing.

Retained the ptrace muck because it's a nice test case, your patch is
obviously a better fix there.

Haven't bothered looking at alternatives yet, this is a
defconfig+tracing build.

Weekend now.

---
 arch/x86/include/asm/special_insns.h |   2 +
 arch/x86/kernel/ptrace.c             |   3 +-
 include/linux/frame.h                |  38 ++++++++++++
 tools/objtool/Makefile               |   2 +-
 tools/objtool/arch.h                 |   6 +-
 tools/objtool/arch/x86/decode.c      |  22 ++++++-
 tools/objtool/check.c                | 117 ++++++++++++++++++++++++++++++++++-
 tools/objtool/check.h                |   2 +-
 tools/objtool/elf.h                  |   1 +
 9 files changed, 187 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 43c029cdc3fe..cd31e4433f4c 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -5,6 +5,7 @@
 
 #ifdef __KERNEL__
 
+#include <linux/frame.h>
 #include <asm/nops.h>
 
 /*
@@ -135,6 +136,7 @@ static inline void native_wbinvd(void)
 }
 
 extern asmlinkage void native_load_gs_index(unsigned);
+AC_SAFE(native_load_gs_index);
 
 static inline unsigned long __read_cr4(void)
 {
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 4b8ee05dd6ad..e278b4055a8b 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -420,7 +420,7 @@ static int putreg(struct task_struct *child,
 	return 0;
 }
 
-static unsigned long getreg(struct task_struct *task, unsigned long offset)
+static notrace unsigned long getreg(struct task_struct *task, unsigned long offset)
 {
 	switch (offset) {
 	case offsetof(struct user_regs_struct, cs):
@@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task, unsigned long offset)
 
 	return *pt_regs_access(task_pt_regs(task), offset);
 }
+AC_SAFE(getreg);
 
 static int genregs_get(struct task_struct *target,
 		       const struct user_regset *regset,
diff --git a/include/linux/frame.h b/include/linux/frame.h
index 02d3ca2d9598..870a894bc586 100644
--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -21,4 +21,42 @@
 
 #endif /* CONFIG_STACK_VALIDATION */
 
+#if defined(CONFIG_STACK_VALIDATION)
+/*
+ * This macro marks functions as AC-safe, that is, it is safe to call from an
+ * EFLAGS.AC enabled region (typically user_access_begin() /
+ * user_access_end()).
+ *
+ * These functions in turn will only call AC-safe functions themselves (which
+ * precludes tracing, including __fentry__ and scheduling, including
+ * preempt_enable).
+ *
+ * AC-safe functions will obviously also not change EFLAGS.AC themselves.
+ */
+#define AC_SAFE(func)						\
+	asm (".pushsection .discard.ac_safe_sym, \"S\", @3\n\t"	\
+	     "999: .ascii \"" #func "\"\n\t"			\
+	     "     .byte 0\n\t"					\
+	     ".popsection\n\t"					\
+	     ".pushsection .discard.ac_safe\n\t"		\
+	     ".long 999b - .\n\t"				\
+	     ".popsection")
+
+/*
+ * SHT_STRTAB sayeth:
+ *
+ * - The initial byte of a string table is \0. This allows an string offset
+ *   value of zero to denote the NULL string.
+ *
+ * Works just fine without this, but since we set the proper section type, lets
+ * not confuse anybody reading this.
+ */
+asm(".pushsection .discard.ac_safe_sym, \"S\", @3\n\t"
+    ".byte 0\n\t"
+    ".popsection\n\t");
+
+#else
+#define AC_SAFE(func)
+#endif
+
 #endif /* _LINUX_FRAME_H */
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index c9d038f91af6..5a64e5fb9d7a 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
 	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
 	    -I$(srctree)/tools/objtool/arch/$(ARCH)/include
 WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
-CFLAGS   += -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES)
+CFLAGS   += -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) -ggdb3
 LDFLAGS  += -lelf $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
 
 # Allow old libelf to be used:
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index b0d7dc3d71b5..9795d83cbc01 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -33,7 +33,11 @@
 #define INSN_STACK		8
 #define INSN_BUG		9
 #define INSN_NOP		10
-#define INSN_OTHER		11
+#define INSN_STAC		11
+#define INSN_CLAC		12
+#define INSN_STD		13
+#define INSN_CLD		14
+#define INSN_OTHER		15
 #define INSN_LAST		INSN_OTHER
 
 enum op_dest_type {
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 540a209b78ab..bee32ad53ecf 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 
 	case 0x0f:
 
-		if (op2 >= 0x80 && op2 <= 0x8f) {
+		if (op2 == 0x01) {
+
+			if (modrm == 0xca) {
+
+				*type = INSN_CLAC;
+
+			} else if (modrm == 0xcb) {
+
+				*type = INSN_STAC;
+
+			}
+
+		} else if (op2 >= 0x80 && op2 <= 0x8f) {
 
 			*type = INSN_JUMP_CONDITIONAL;
 
@@ -444,6 +456,14 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 		*type = INSN_CALL;
 		break;
 
+	case 0xfc:
+		*type = INSN_CLD;
+		break;
+
+	case 0xfd:
+		*type = INSN_STD;
+		break;
+
 	case 0xff:
 		if (modrm_reg == 2 || modrm_reg == 3)
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0414a0d52262..3dedfa19cb09 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -451,6 +451,41 @@ static void add_ignores(struct objtool_file *file)
 	}
 }
 
+static int add_ac_safe(struct objtool_file *file)
+{
+	struct section *sec_sym, *sec;
+	struct symbol *func;
+	struct rela *rela;
+	const char *name;
+
+	sec = find_section_by_name(file->elf, ".rela.discard.ac_safe");
+	if (!sec)
+		return 0;
+
+	sec_sym = find_section_by_name(file->elf, ".discard.ac_safe_sym");
+	if (!sec_sym) {
+		WARN("missing AC-safe symbols");
+		return -1;
+	}
+
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		if (rela->sym->type != STT_SECTION) {
+			WARN("unexpected relocation symbol type in %s", sec->name);
+			return -1;
+		}
+
+		name = elf_strptr(file->elf->elf, sec_sym->idx, rela->addend);
+
+		func = find_symbol_by_name(file->elf, name);
+		if (!func)
+			continue;
+
+		func->ac_safe = true;
+	}
+
+	return 0;
+}
+
 /*
  * FIXME: For now, just ignore any alternatives which add retpolines.  This is
  * a temporary hack, as it doesn't allow ORC to unwind from inside a retpoline.
@@ -695,6 +730,7 @@ static int handle_group_alt(struct objtool_file *file,
 		last_new_insn = insn;
 
 		insn->ignore = orig_insn->ignore_alts;
+		insn->func = orig_insn->func;
 
 		if (insn->type != INSN_JUMP_CONDITIONAL &&
 		    insn->type != INSN_JUMP_UNCONDITIONAL)
@@ -1239,6 +1275,10 @@ static int decode_sections(struct objtool_file *file)
 
 	add_ignores(file);
 
+	ret = add_ac_safe(file);
+	if (ret)
+		return ret;
+
 	ret = add_nospec_ignores(file);
 	if (ret)
 		return ret;
@@ -1902,6 +1942,15 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 		switch (insn->type) {
 
 		case INSN_RETURN:
+			if (state.ac) {
+				WARN_FUNC("return with AC set", sec, insn->offset);
+				return 1;
+			}
+			if (state.df) {
+				WARN_FUNC("return with DF set", sec, insn->offset);
+				return 1;
+			}
+
 			if (func && has_modified_stack_frame(&state)) {
 				WARN_FUNC("return with modified stack frame",
 					  sec, insn->offset);
@@ -1917,6 +1966,17 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 			return 0;
 
 		case INSN_CALL:
+			if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
+				WARN_FUNC("call to %s() with AC set", sec, insn->offset,
+						insn->call_dest->name);
+				return 1;
+			}
+			if (state.df) {
+				WARN_FUNC("call to %s() with DF set", sec, insn->offset,
+						insn->call_dest->name);
+				return 1;
+			}
+
 			if (is_fentry_call(insn))
 				break;
 
@@ -1926,8 +1986,25 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 			if (ret == -1)
 				return 1;
 
-			/* fallthrough */
+			if (!no_fp && func && !has_valid_stack_frame(&state)) {
+				WARN_FUNC("call without frame pointer save/setup",
+					  sec, insn->offset);
+				return 1;
+			}
+			break;
+
 		case INSN_CALL_DYNAMIC:
+			if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
+				WARN_FUNC("call to %s() with AC set", sec, insn->offset,
+						insn->call_dest->name);
+				return 1;
+			}
+			if (state.df) {
+				WARN_FUNC("call to %s() with DF set", sec, insn->offset,
+						insn->call_dest->name);
+				return 1;
+			}
+
 			if (!no_fp && func && !has_valid_stack_frame(&state)) {
 				WARN_FUNC("call without frame pointer save/setup",
 					  sec, insn->offset);
@@ -1980,6 +2057,42 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 
 			break;
 
+		case INSN_STAC:
+			if (state.ac_safe || state.ac) {
+				WARN_FUNC("recursive STAC", sec, insn->offset);
+				return 1;
+			}
+			state.ac = true;
+			break;
+
+		case INSN_CLAC:
+			if (!state.ac && insn->func) {
+				WARN_FUNC("redundant CLAC", sec, insn->offset);
+				return 1;
+			}
+			if (state.ac_safe) {
+				WARN_FUNC("AC-safe clears AC", sec, insn->offset);
+				return 1;
+			}
+			state.ac = false;
+			break;
+
+		case INSN_STD:
+			if (state.df) {
+				WARN_FUNC("recursive STD", sec, insn->offset);
+				return 1;
+			}
+			state.df = true;
+			break;
+
+		case INSN_CLD:
+			if (!state.df && insn->func) {
+				WARN_FUNC("redundant CLD", sec, insn->offset);
+				return 1;
+			}
+			state.df = false;
+			break;
+
 		default:
 			break;
 		}
@@ -2141,6 +2254,8 @@ static int validate_functions(struct objtool_file *file)
 			if (!insn || insn->ignore)
 				continue;
 
+			state.ac_safe = func->ac_safe;
+
 			ret = validate_branch(file, insn, state);
 			warnings += ret;
 		}
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index e6e8a655b556..602634792151 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -31,7 +31,7 @@ struct insn_state {
 	int stack_size;
 	unsigned char type;
 	bool bp_scratch;
-	bool drap, end;
+	bool drap, end, ac, ac_safe, df;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];
 };
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index bc97ed86b9cd..064c3df31e40 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -62,6 +62,7 @@ struct symbol {
 	unsigned long offset;
 	unsigned int len;
 	struct symbol *pfunc, *cfunc;
+	bool ac_safe;
 };
 
 struct rela {

Julien Thierry Feb. 25, 2019, 8:33 a.m. UTC | #13

On 22/02/2019 22:26, Peter Zijlstra wrote:
> On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
> 
>> But correct :)
> 
>> I agree, that a function which is doing the actual copy should be callable,
>> but random other functions? NO!
> 
> So find the below patch -- which spotted fail in ptrace.c
> 
> It has an AC_SAFE(func) annotation which allows marking specific
> functions as safe to call. The patch includes 2 instances which were
> required to make arch/x86 'build':
> 
>   arch/x86/ia32/ia32_signal.o: warning: objtool: ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC set
>   arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to getreg() with AC set
> 
> It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the
> lack of notrace annotations on functions marked AC_SAFE():
> 
>   arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to __fentry__() with AC set
> 
> It builds arch/x86 relatively clean; it only complains about some
> redundant CLACs in entry_64.S because it doesn't understand interrupts
> and I've not bothered creating an annotation for them yet.
> 
>   arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x4d: redundant CLAC
>   arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x5a: redundant CLAC
>   ...
>   arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC
> 
> Also, I realized we don't need special annotations for preempt_count;
> preempt_disable() emits a CALL instruction which should readily trigger
> the warnings added here.
> 
> The VDSO thing is a bit of a hack, but I couldn't quickly find anything
> better.
> 
> Comments?

I haven't looked at all the details. But could the annotation be called
UACCESS_SAFE() (and corresponding naming in the objtool checks)? Since
this is not an x86 only issue and the AC flags only exists for x86.

Cheers,

Julien

> 
> ---
>  arch/x86/include/asm/special_insns.h |  2 ++
>  arch/x86/kernel/ptrace.c             |  3 +-
>  include/linux/frame.h                | 23 ++++++++++++++
>  tools/objtool/arch.h                 |  4 ++-
>  tools/objtool/arch/x86/decode.c      | 14 ++++++++-
>  tools/objtool/check.c                | 59 ++++++++++++++++++++++++++++++++++++
>  tools/objtool/check.h                |  3 +-
>  tools/objtool/elf.h                  |  1 +
>  8 files changed, 105 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 43c029cdc3fe..cd31e4433f4c 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -5,6 +5,7 @@
>  
>  #ifdef __KERNEL__
>  
> +#include <linux/frame.h>
>  #include <asm/nops.h>
>  
>  /*
> @@ -135,6 +136,7 @@ static inline void native_wbinvd(void)
>  }
>  
>  extern asmlinkage void native_load_gs_index(unsigned);
> +AC_SAFE(native_load_gs_index);
>  
>  static inline unsigned long __read_cr4(void)
>  {
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 4b8ee05dd6ad..e278b4055a8b 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -420,7 +420,7 @@ static int putreg(struct task_struct *child,
>  	return 0;
>  }
>  
> -static unsigned long getreg(struct task_struct *task, unsigned long offset)
> +static notrace unsigned long getreg(struct task_struct *task, unsigned long offset)
>  {
>  	switch (offset) {
>  	case offsetof(struct user_regs_struct, cs):
> @@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task, unsigned long offset)
>  
>  	return *pt_regs_access(task_pt_regs(task), offset);
>  }
> +AC_SAFE(getreg);
>  
>  static int genregs_get(struct task_struct *target,
>  		       const struct user_regset *regset,
> diff --git a/include/linux/frame.h b/include/linux/frame.h
> index 02d3ca2d9598..5d354cf42a56 100644
> --- a/include/linux/frame.h
> +++ b/include/linux/frame.h
> @@ -21,4 +21,27 @@
>  
>  #endif /* CONFIG_STACK_VALIDATION */
>  
> +#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) && !defined(BUILD_VDSO32)
> +/*
> + * This macro marks functions as AC-safe, that is, it is safe to call from an
> + * EFLAGS.AC enabled region (typically user_access_begin() /
> + * user_access_end()).
> + *
> + * These functions in turn will only call AC-safe functions themselves (which
> + * precludes tracing, including __fentry__ and scheduling, including
> + * preempt_enable).
> + *
> + * AC-safe functions will obviously also not change EFLAGS.AC themselves.
> + *
> + * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO builds
> + * (and the generated symbol reference will in fact cause link failures).
> + */
> +#define AC_SAFE(func) \
> +	static void __used __section(.discard.ac_safe) \
> +		*__func_ac_safe_##func = func
> +
> +#else
> +#define AC_SAFE(func)
> +#endif
> +
>  #endif /* _LINUX_FRAME_H */
> diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
> index b0d7dc3d71b5..48327099466d 100644
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -33,7 +33,9 @@
>  #define INSN_STACK		8
>  #define INSN_BUG		9
>  #define INSN_NOP		10
> -#define INSN_OTHER		11
> +#define INSN_STAC		11
> +#define INSN_CLAC		12
> +#define INSN_OTHER		13
>  #define INSN_LAST		INSN_OTHER
>  
>  enum op_dest_type {
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index 540a209b78ab..d1e99d1460a5 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>  
>  	case 0x0f:
>  
> -		if (op2 >= 0x80 && op2 <= 0x8f) {
> +		if (op2 == 0x01) {
> +
> +			if (modrm == 0xca) {
> +
> +				*type = INSN_CLAC;
> +
> +			} else if (modrm == 0xcb) {
> +
> +				*type = INSN_STAC;
> +
> +			}
> +
> +		} else if (op2 >= 0x80 && op2 <= 0x8f) {
>  
>  			*type = INSN_JUMP_CONDITIONAL;
>  
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 0414a0d52262..01852602ca31 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -127,6 +127,24 @@ static bool ignore_func(struct objtool_file *file, struct symbol *func)
>  	return false;
>  }
>  
> +static bool ac_safe_func(struct objtool_file *file, struct symbol *func)
> +{
> +	struct rela *rela;
> +
> +	/* check for AC_SAFE */
> +	if (file->ac_safe && file->ac_safe->rela)
> +		list_for_each_entry(rela, &file->ac_safe->rela->rela_list, list) {
> +			if (rela->sym->type == STT_SECTION &&
> +			    rela->sym->sec == func->sec &&
> +			    rela->addend == func->offset)
> +				return true;
> +			if (/* rela->sym->type == STT_FUNC && */ rela->sym == func)
> +				return true;
> +		}
> +
> +	return false;
> +}
> +
>  /*
>   * This checks to see if the given function is a "noreturn" function.
>   *
> @@ -439,6 +457,8 @@ static void add_ignores(struct objtool_file *file)
>  
>  	for_each_sec(file, sec) {
>  		list_for_each_entry(func, &sec->symbol_list, list) {
> +			func->ac_safe = ac_safe_func(file, func);
> +
>  			if (func->type != STT_FUNC)
>  				continue;
>  
> @@ -1902,6 +1922,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
>  		switch (insn->type) {
>  
>  		case INSN_RETURN:
> +			if (state.ac) {
> +				WARN_FUNC("return with AC set", sec, insn->offset);
> +				return 1;
> +			}
> +
>  			if (func && has_modified_stack_frame(&state)) {
>  				WARN_FUNC("return with modified stack frame",
>  					  sec, insn->offset);
> @@ -1917,6 +1942,12 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
>  			return 0;
>  
>  		case INSN_CALL:
> +			if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
> +				WARN_FUNC("call to %s() with AC set", sec, insn->offset,
> +						insn->call_dest->name);
> +				return 1;
> +			}
> +
>  			if (is_fentry_call(insn))
>  				break;
>  
> @@ -1928,6 +1959,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
>  
>  			/* fallthrough */
>  		case INSN_CALL_DYNAMIC:
> +			if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
> +				WARN_FUNC("call to %s() with AC set", sec, insn->offset,
> +						insn->call_dest->name);
> +				return 1;
> +			}
>  			if (!no_fp && func && !has_valid_stack_frame(&state)) {
>  				WARN_FUNC("call without frame pointer save/setup",
>  					  sec, insn->offset);
> @@ -1980,6 +2016,26 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
>  
>  			break;
>  
> +		case INSN_STAC:
> +			if (state.ac_safe || state.ac) {
> +				WARN_FUNC("recursive STAC", sec, insn->offset);
> +				return 1;
> +			}
> +			state.ac = true;
> +			break;
> +
> +		case INSN_CLAC:
> +			if (!state.ac) {
> +				WARN_FUNC("redundant CLAC", sec, insn->offset);
> +				return 1;
> +			}
> +			if (state.ac_safe) {
> +				WARN_FUNC("AC-safe clears AC", sec, insn->offset);
> +				return 1;
> +			}
> +			state.ac = false;
> +			break;
> +
>  		default:
>  			break;
>  		}
> @@ -2141,6 +2197,8 @@ static int validate_functions(struct objtool_file *file)
>  			if (!insn || insn->ignore)
>  				continue;
>  
> +			state.ac_safe = func->ac_safe;
> +
>  			ret = validate_branch(file, insn, state);
>  			warnings += ret;
>  		}
> @@ -2198,6 +2256,7 @@ int check(const char *_objname, bool orc)
>  	INIT_LIST_HEAD(&file.insn_list);
>  	hash_init(file.insn_hash);
>  	file.whitelist = find_section_by_name(file.elf, ".discard.func_stack_frame_non_standard");
> +	file.ac_safe = find_section_by_name(file.elf, ".discard.ac_safe");
>  	file.c_file = find_section_by_name(file.elf, ".comment");
>  	file.ignore_unreachables = no_unreachable;
>  	file.hints = false;
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index e6e8a655b556..c31ec3ca78f3 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -31,7 +31,7 @@ struct insn_state {
>  	int stack_size;
>  	unsigned char type;
>  	bool bp_scratch;
> -	bool drap, end;
> +	bool drap, end, ac, ac_safe;
>  	int drap_reg, drap_offset;
>  	struct cfi_reg vals[CFI_NUM_REGS];
>  };
> @@ -61,6 +61,7 @@ struct objtool_file {
>  	struct list_head insn_list;
>  	DECLARE_HASHTABLE(insn_hash, 16);
>  	struct section *whitelist;
> +	struct section *ac_safe;
>  	bool ignore_unreachables, c_file, hints, rodata;
>  };
>  
> diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
> index bc97ed86b9cd..064c3df31e40 100644
> --- a/tools/objtool/elf.h
> +++ b/tools/objtool/elf.h
> @@ -62,6 +62,7 @@ struct symbol {
>  	unsigned long offset;
>  	unsigned int len;
>  	struct symbol *pfunc, *cfunc;
> +	bool ac_safe;
>  };
>  
>  struct rela {
>

H. Peter Anvin Feb. 25, 2019, 8:47 a.m. UTC | #14

On February 23, 2019 12:39:42 AM PST, Peter Zijlstra <[e-mail]> wrote:
>On Fri, Feb 22, 2019 at 03:39:48PM -0800, [e-mail] wrote:
>> Objtool could also detect CLAC-STAC or STAC-CLAC sequences without
>> memory operations and remove them; don't know how often that happens,
>> but I know it *does* happen.
>
>Objtool doesn't know about memops; that'd be a lot of work. Also,
>objtool doesn't actually rewrite the text, at best it could warn about
>such occurences.

It doesn't have to understand the contents of the memop, but it seems that the presence of a modrm with mode ≠ 3 should be plenty. It needs to know that much in order to know the length of instructions anyway. For extra credit, ignore LEA or hinting instructions.

H. Peter Anvin Feb. 25, 2019, 8:49 a.m. UTC | #15

On February 23, 2019 12:39:42 AM PST, Peter Zijlstra <[e-mail]> wrote:
>On Fri, Feb 22, 2019 at 03:39:48PM -0800, [e-mail] wrote:
>> Objtool could also detect CLAC-STAC or STAC-CLAC sequences without
>> memory operations and remove them; don't know how often that happens,
>> but I know it *does* happen.
>
>Objtool doesn't know about memops; that'd be a lot of work. Also,
>objtool doesn't actually rewrite the text, at best it could warn about
>such occurences.

It doesn't even have to change the text per se, just nullify the altinstr.

Peter Zijlstra Feb. 25, 2019, 10:51 a.m. UTC | #16

On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote:
> I'm wondering if we can just change the code that does getreg() and
> load_gs_index() so it doesn't do it with AC set.  Also, what about
> paravirt kernels?  They'll call into PV code for load_gs_index() with
> AC set.

Paravirt can go bugger off. There's no sane way to fix that.

Luckily the load_gs_index() thing is part of the paravirt me harder crap
and so nobody sane should ever hit that.

I don't fully understand that code at all; I also have no clue why GS
has paravirt bits on but the other segments do not. But it looks like we
want to do that RELOAD_SEG() crap under SMAP because of the GET_SEG() ->
get_user_ex() thing.

Anyway, I only see 3 options here:

 1) live with the paravirt me harder builds complaining
 2) exclude the AC validation from the paravirt me harder builds
 3) rewrite this code to no need that stupid call in the first place


2 seems like an exceptionally bad ideal, 3 would need someone that
understands this, so for now I'll pick 1 :-)


*thought*... we could delay the actual set_user_seg() thing until after
the get_user_catch(), would that work?

Peter Zijlstra Feb. 25, 2019, 11:53 a.m. UTC | #17

On Mon, Feb 25, 2019 at 11:51:44AM +0100, Peter Zijlstra wrote:
> On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote:
> > I'm wondering if we can just change the code that does getreg() and
> > load_gs_index() so it doesn't do it with AC set.  Also, what about
> > paravirt kernels?  They'll call into PV code for load_gs_index() with
> > AC set.
> 
> Paravirt can go bugger off. There's no sane way to fix that.

> I don't fully understand that code at all; I also have no clue why GS
> has paravirt bits on but the other segments do not. 

*sigh* SWAPGS

> *thought*... we could delay the actual set_user_seg() thing until after
> the get_user_catch(), would that work?


How horrible / broken is this?

---

diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 321fe5f5d0e9..67c866943102 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -60,17 +60,21 @@
 	regs->seg = GET_SEG(seg) | 3;			\
 } while (0)
 
-#define RELOAD_SEG(seg)		{		\
-	unsigned int pre = GET_SEG(seg);	\
-	unsigned int cur = get_user_seg(seg);	\
-	pre |= 3;				\
-	if (pre != cur)				\
-		set_user_seg(seg, pre);		\
+#define LOAD_SEG(seg)		{			\
+	pre_##seg = 3 | GET_SEG(seg);			\
+	cur_##seg = get_user_seg(seg);			\
+}
+
+#define RELOAD_SEG(seg)		{			\
+	if (pre_##seg != cur_##seg)			\
+		set_user_seg(seg, pre_##seg);		\
 }
 
 static int ia32_restore_sigcontext(struct pt_regs *regs,
 				   struct sigcontext_32 __user *sc)
 {
+	u16 pre_gs, pre_fs, pre_ds, pre_es;
+	u16 cur_gs, cur_fs, cur_ds, cur_es;
 	unsigned int tmpflags, err = 0;
 	void __user *buf;
 	u32 tmp;
@@ -85,10 +89,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 		 * the handler, but does not clobber them at least in the
 		 * normal case.
 		 */
-		RELOAD_SEG(gs);
-		RELOAD_SEG(fs);
-		RELOAD_SEG(ds);
-		RELOAD_SEG(es);
+		LOAD_SEG(gs);
+		LOAD_SEG(fs);
+		LOAD_SEG(ds);
+		LOAD_SEG(es);
 
 		COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
 		COPY(dx); COPY(cx); COPY(ip); COPY(ax);
@@ -106,6 +110,11 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 		buf = compat_ptr(tmp);
 	} get_user_catch(err);
 
+	RELOAD_SEG(gs);
+	RELOAD_SEG(fs);
+	RELOAD_SEG(ds);
+	RELOAD_SEG(es);
+
 	err |= fpu__restore_sig(buf, 1);
 
 	force_iret();

Peter Zijlstra Feb. 25, 2019, 11:55 a.m. UTC | #18

On Mon, Feb 25, 2019 at 08:33:35AM +0000, Julien Thierry wrote:
> > It has an AC_SAFE(func) annotation which allows marking specific
> > functions as safe to call. The patch includes 2 instances which were
> > required to make arch/x86 'build':

> I haven't looked at all the details. But could the annotation be called
> UACCESS_SAFE() (and corresponding naming in the objtool checks)? Since
> this is not an x86 only issue and the AC flags only exists for x86.

Sure that works. Lemme sed the patches.

Peter Zijlstra Feb. 25, 2019, 1:21 p.m. UTC | #19

On Mon, Feb 25, 2019 at 12:47:00AM -0800, [e-mail] wrote:
> It doesn't have to understand the contents of the memop, but it seems
> that the presence of a modrm with mode ≠ 3 should be plenty. It needs
> to know that much in order to know the length of instructions anyway.
> For extra credit, ignore LEA or hinting instructions.

A little something like so then?

arch/x86/kernel/fpu/signal.o: warning: objtool: .altinstr_replacement+0x9c: UACCESS disable without MEMOPs: copy_fpstate_to_sigframe()

00000000023c  000200000002 R_X86_64_PC32     0000000000000000 .text + 604
000000000240  000700000002 R_X86_64_PC32     0000000000000000 .altinstr_replacement + 99
000000000249  000200000002 R_X86_64_PC32     0000000000000000 .text + 610
00000000024d  000700000002 R_X86_64_PC32     0000000000000000 .altinstr_replacement + 9c

.text

604:   90                      nop
605:   90                      nop
606:   90                      nop
607:   83 ce 03                or     $0x3,%esi
60a:   89 b3 00 02 00 00       mov    %esi,0x200(%rbx)
610:   90                      nop
611:   90                      nop
612:   90                      nop

.altinstr_replacement

99:   0f 01 cb                stac
9c:   0f 01 ca                clac

Which looks like the tool failed to recognise that MOV as a memop.



---
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -37,7 +37,8 @@
 #define INSN_CLAC		12
 #define INSN_STD		13
 #define INSN_CLD		14
-#define INSN_OTHER		15
+#define INSN_MEMOP		15
+#define INSN_OTHER		16
 #define INSN_LAST		INSN_OTHER
 
 enum op_dest_type {
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -123,6 +123,9 @@ int arch_decode_instruction(struct elf *
 		modrm_mod = X86_MODRM_MOD(modrm);
 		modrm_reg = X86_MODRM_REG(modrm);
 		modrm_rm = X86_MODRM_RM(modrm);
+
+		if (modrm_mod != 3)
+			*type = INSN_MEMOP;
 	}
 
 	if (insn.sib.nbytes)
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2047,6 +2047,7 @@ static int validate_branch(struct objtoo
 				WARN_FUNC("recursive UACCESS enable", sec, insn->offset);
 
 			state.uaccess = true;
+			state.memop = false;
 			break;
 
 		case INSN_CLAC:
@@ -2058,6 +2059,9 @@ static int validate_branch(struct objtoo
 				break;
 			}
 
+			if (!state.memop && insn->func)
+				WARN_FUNC("UACCESS disable without MEMOPs: %s()", sec, insn->offset, insn->func->name);
+
 			state.uaccess = false;
 			break;
 
@@ -2075,6 +2079,10 @@ static int validate_branch(struct objtoo
 			state.df = false;
 			break;
 
+		case INSN_MEMOP:
+			state.memop = true;
+			break;
+
 		default:
 			break;
 		}
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -31,7 +31,7 @@ struct insn_state {
 	int stack_size;
 	unsigned char type;
 	bool bp_scratch;
-	bool drap, end, uaccess, uaccess_safe, df;
+	bool drap, end, uaccess, uaccess_safe, df, memop;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];
 };

Andy Lutomirski Feb. 25, 2019, 3:36 p.m. UTC | #20

> On Feb 25, 2019, at 3:53 AM, Peter Zijlstra <[e-mail]> wrote:
> 
>> On Mon, Feb 25, 2019 at 11:51:44AM +0100, Peter Zijlstra wrote:
>>> On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote:
>>> I'm wondering if we can just change the code that does getreg() and
>>> load_gs_index() so it doesn't do it with AC set.  Also, what about
>>> paravirt kernels?  They'll call into PV code for load_gs_index() with
>>> AC set.
>> 
>> Paravirt can go bugger off. There's no sane way to fix that.
> 
>> I don't fully understand that code at all; I also have no clue why GS
>> has paravirt bits on but the other segments do not. 
> 
> *sigh* SWAPGS
> 
>> *thought*... we could delay the actual set_user_seg() thing until after
>> the get_user_catch(), would that work?
> 
> 
> How horrible / broken is this?
> 
> ---
> 
> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
> index 321fe5f5d0e9..67c866943102 100644
> --- a/arch/x86/ia32/ia32_signal.c
> +++ b/arch/x86/ia32/ia32_signal.c
> @@ -60,17 +60,21 @@
>    regs->seg = GET_SEG(seg) | 3;            \
> } while (0)
> 
> -#define RELOAD_SEG(seg)        {        \
> -    unsigned int pre = GET_SEG(seg);    \
> -    unsigned int cur = get_user_seg(seg);    \
> -    pre |= 3;                \
> -    if (pre != cur)                \
> -        set_user_seg(seg, pre);        \
> +#define LOAD_SEG(seg)        {            \
> +    pre_##seg = 3 | GET_SEG(seg);            \
> +    cur_##seg = get_user_seg(seg);            \
> +}
> +
> +#define RELOAD_SEG(seg)        {            \
> +    if (pre_##seg != cur_##seg)            \
> +        set_user_seg(seg, pre_##seg);        \
> }
> 
> static int ia32_restore_sigcontext(struct pt_regs *regs,
>                   struct sigcontext_32 __user *sc)
> {
> +    u16 pre_gs, pre_fs, pre_ds, pre_es;
> +    u16 cur_gs, cur_fs, cur_ds, cur_es;
>    unsigned int tmpflags, err = 0;
>    void __user *buf;
>    u32 tmp;
> @@ -85,10 +89,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>         * the handler, but does not clobber them at least in the
>         * normal case.
>         */
> -        RELOAD_SEG(gs);
> -        RELOAD_SEG(fs);
> -        RELOAD_SEG(ds);
> -        RELOAD_SEG(es);
> +        LOAD_SEG(gs);
> +        LOAD_SEG(fs);
> +        LOAD_SEG(ds);
> +        LOAD_SEG(es);
> 
>        COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
>        COPY(dx); COPY(cx); COPY(ip); COPY(ax);
> @@ -106,6 +110,11 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>        buf = compat_ptr(tmp);
>    } get_user_catch(err);
> 
> +    RELOAD_SEG(gs);
> +    RELOAD_SEG(fs);
> +    RELOAD_SEG(ds);
> +    RELOAD_SEG(es);
> +
>    err |= fpu__restore_sig(buf, 1);
> 
>    force_iret();

I would call this pretty horrible. How about we do it without macros? :)

But yes, deferring the segment load until after the read seems fine to me. Frankly, we could also just copy_from_user the whole thing up front — thus code is not really a serious fast path.

Peter Zijlstra March 1, 2019, 3:07 p.m. UTC | #21

On Mon, Feb 25, 2019 at 02:21:03PM +0100, Peter Zijlstra wrote:
> On Mon, Feb 25, 2019 at 12:47:00AM -0800, [e-mail] wrote:
> > It doesn't have to understand the contents of the memop, but it seems
> > that the presence of a modrm with mode ≠ 3 should be plenty. It needs
> > to know that much in order to know the length of instructions anyway.
> > For extra credit, ignore LEA or hinting instructions.
> 
> A little something like so then?


$ ./objtool check --no-fp --backtrace ../../defconfig-build/arch/x86/lib/usercopy_64.o
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x3: UACCESS disable without MEMOPs: __clear_user()
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x3a: (alt)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x2e: (branch)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x18: (branch)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0xffffffffffffffff: (branch)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x5: (alt)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x0: <=== (func)


0000000000000000 <__clear_user>:
   0:   e8 00 00 00 00          callq  5 <__clear_user+0x5>
   1: R_X86_64_PLT32       __fentry__-0x4
   5:   90                      nop
   6:   90                      nop
   7:   90                      nop
   8:   48 89 f0                mov    %rsi,%rax
   b:   48 c1 ee 03             shr    $0x3,%rsi
   f:   83 e0 07                and    $0x7,%eax
  12:   48 89 f1                mov    %rsi,%rcx
  15:   48 85 c9                test   %rcx,%rcx
  18:   74 0f                   je     29 <__clear_user+0x29>
  1a:   48 c7 07 00 00 00 00    movq   $0x0,(%rdi)
  21:   48 83 c7 08             add    $0x8,%rdi
  25:   ff c9                   dec    %ecx
  27:   75 f1                   jne    1a <__clear_user+0x1a>
  29:   48 89 c1                mov    %rax,%rcx
  2c:   85 c9                   test   %ecx,%ecx
  2e:   74 0a                   je     3a <__clear_user+0x3a>
  30:   c6 07 00                movb   $0x0,(%rdi)
  33:   48 ff c7                inc    %rdi
  36:   ff c9                   dec    %ecx
  38:   75 f6                   jne    30 <__clear_user+0x30>
  3a:   90                      nop
  3b:   90                      nop
  3c:   90                      nop
  3d:   48 89 c8                mov    %rcx,%rax
  40:   c3                      retq


Seems correct. Not sure you want to go fix that though. Let me know if
you want more output.

Patch

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 43c029cdc3fe..cd31e4433f4c 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -5,6 +5,7 @@ 
 
 #ifdef __KERNEL__
 
+#include <linux/frame.h>
 #include <asm/nops.h>
 
 /*
@@ -135,6 +136,7 @@  static inline void native_wbinvd(void)
 }
 
 extern asmlinkage void native_load_gs_index(unsigned);
+AC_SAFE(native_load_gs_index);
 
 static inline unsigned long __read_cr4(void)
 {
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 4b8ee05dd6ad..e278b4055a8b 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -420,7 +420,7 @@  static int putreg(struct task_struct *child,
 	return 0;
 }
 
-static unsigned long getreg(struct task_struct *task, unsigned long offset)
+static notrace unsigned long getreg(struct task_struct *task, unsigned long offset)
 {
 	switch (offset) {
 	case offsetof(struct user_regs_struct, cs):
@@ -444,6 +444,7 @@  static unsigned long getreg(struct task_struct *task, unsigned long offset)
 
 	return *pt_regs_access(task_pt_regs(task), offset);
 }
+AC_SAFE(getreg);
 
 static int genregs_get(struct task_struct *target,
 		       const struct user_regset *regset,
diff --git a/include/linux/frame.h b/include/linux/frame.h
index 02d3ca2d9598..5d354cf42a56 100644
--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -21,4 +21,27 @@ 
 
 #endif /* CONFIG_STACK_VALIDATION */
 
+#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) && !defined(BUILD_VDSO32)
+/*
+ * This macro marks functions as AC-safe, that is, it is safe to call from an
+ * EFLAGS.AC enabled region (typically user_access_begin() /
+ * user_access_end()).
+ *
+ * These functions in turn will only call AC-safe functions themselves (which
+ * precludes tracing, including __fentry__ and scheduling, including
+ * preempt_enable).
+ *
+ * AC-safe functions will obviously also not change EFLAGS.AC themselves.
+ *
+ * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO builds
+ * (and the generated symbol reference will in fact cause link failures).
+ */
+#define AC_SAFE(func) \
+	static void __used __section(.discard.ac_safe) \
+		*__func_ac_safe_##func = func
+
+#else
+#define AC_SAFE(func)
+#endif
+
 #endif /* _LINUX_FRAME_H */
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index b0d7dc3d71b5..48327099466d 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -33,7 +33,9 @@ 
 #define INSN_STACK		8
 #define INSN_BUG		9
 #define INSN_NOP		10
-#define INSN_OTHER		11
+#define INSN_STAC		11
+#define INSN_CLAC		12
+#define INSN_OTHER		13
 #define INSN_LAST		INSN_OTHER
 
 enum op_dest_type {
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 540a209b78ab..d1e99d1460a5 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -369,7 +369,19 @@  int arch_decode_instruction(struct elf *elf, struct section *sec,
 
 	case 0x0f:
 
-		if (op2 >= 0x80 && op2 <= 0x8f) {
+		if (op2 == 0x01) {
+
+			if (modrm == 0xca) {
+
+				*type = INSN_CLAC;
+
+			} else if (modrm == 0xcb) {
+
+				*type = INSN_STAC;
+
+			}
+
+		} else if (op2 >= 0x80 && op2 <= 0x8f) {
 
 			*type = INSN_JUMP_CONDITIONAL;
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0414a0d52262..01852602ca31 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -127,6 +127,24 @@  static bool ignore_func(struct objtool_file *file, struct symbol *func)
 	return false;
 }
 
+static bool ac_safe_func(struct objtool_file *file, struct symbol *func)
+{
+	struct rela *rela;
+
+	/* check for AC_SAFE */
+	if (file->ac_safe && file->ac_safe->rela)
+		list_for_each_entry(rela, &file->ac_safe->rela->rela_list, list) {
+			if (rela->sym->type == STT_SECTION &&
+			    rela->sym->sec == func->sec &&
+			    rela->addend == func->offset)
+				return true;
+			if (/* rela->sym->type == STT_FUNC && */ rela->sym == func)
+				return true;
+		}
+
+	return false;
+}
+
 /*
  * This checks to see if the given function is a "noreturn" function.
  *
@@ -439,6 +457,8 @@  static void add_ignores(struct objtool_file *file)
 
 	for_each_sec(file, sec) {
 		list_for_each_entry(func, &sec->symbol_list, list) {
+			func->ac_safe = ac_safe_func(file, func);
+
 			if (func->type != STT_FUNC)
 				continue;
 
@@ -1902,6 +1922,11 @@  static int validate_branch(struct objtool_file *file, struct instruction *first,
 		switch (insn->type) {
 
 		case INSN_RETURN:
+			if (state.ac) {
+				WARN_FUNC("return with AC set", sec, insn->offset);
+				return 1;
+			}
+
 			if (func && has_modified_stack_frame(&state)) {
 				WARN_FUNC("return with modified stack frame",
 					  sec, insn->offset);
@@ -1917,6 +1942,12 @@  static int validate_branch(struct objtool_file *file, struct instruction *first,
 			return 0;
 
 		case INSN_CALL:
+			if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
+				WARN_FUNC("call to %s() with AC set", sec, insn->offset,
+						insn->call_dest->name);
+				return 1;
+			}
+
 			if (is_fentry_call(insn))
 				break;
 
@@ -1928,6 +1959,11 @@  static int validate_branch(struct objtool_file *file, struct instruction *first,
 
 			/* fallthrough */
 		case INSN_CALL_DYNAMIC:
+			if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
+				WARN_FUNC("call to %s() with AC set", sec, insn->offset,
+						insn->call_dest->name);
+				return 1;
+			}
 			if (!no_fp && func && !has_valid_stack_frame(&state)) {
 				WARN_FUNC("call without frame pointer save/setup",
 					  sec, insn->offset);
@@ -1980,6 +2016,26 @@  static int validate_branch(struct objtool_file *file, struct instruction *first,
 
 			break;
 
+		case INSN_STAC:
+			if (state.ac_safe || state.ac) {
+				WARN_FUNC("recursive STAC", sec, insn->offset);
+				return 1;
+			}
+			state.ac = true;
+			break;
+
+		case INSN_CLAC:
+			if (!state.ac) {
+				WARN_FUNC("redundant CLAC", sec, insn->offset);
+				return 1;
+			}
+			if (state.ac_safe) {
+				WARN_FUNC("AC-safe clears AC", sec, insn->offset);
+				return 1;
+			}
+			state.ac = false;
+			break;
+
 		default:
 			break;
 		}
@@ -2141,6 +2197,8 @@  static int validate_functions(struct objtool_file *file)
 			if (!insn || insn->ignore)
 				continue;
 
+			state.ac_safe = func->ac_safe;
+
 			ret = validate_branch(file, insn, state);
 			warnings += ret;
 		}
@@ -2198,6 +2256,7 @@  int check(const char *_objname, bool orc)
 	INIT_LIST_HEAD(&file.insn_list);
 	hash_init(file.insn_hash);
 	file.whitelist = find_section_by_name(file.elf, ".discard.func_stack_frame_non_standard");
+	file.ac_safe = find_section_by_name(file.elf, ".discard.ac_safe");
 	file.c_file = find_section_by_name(file.elf, ".comment");
 	file.ignore_unreachables = no_unreachable;
 	file.hints = false;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index e6e8a655b556..c31ec3ca78f3 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -31,7 +31,7 @@  struct insn_state {
 	int stack_size;
 	unsigned char type;
 	bool bp_scratch;
-	bool drap, end;
+	bool drap, end, ac, ac_safe;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];
 };
@@ -61,6 +61,7 @@  struct objtool_file {
 	struct list_head insn_list;
 	DECLARE_HASHTABLE(insn_hash, 16);
 	struct section *whitelist;
+	struct section *ac_safe;
 	bool ignore_unreachables, c_file, hints, rodata;
 };
 
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index bc97ed86b9cd..064c3df31e40 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -62,6 +62,7 @@  struct symbol {
 	unsigned long offset;
 	unsigned int len;
 	struct symbol *pfunc, *cfunc;
+	bool ac_safe;
 };
 
 struct rela {