linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Julien Thierry <julien.thierry@arm.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Will Deacon <will.deacon@arm.com>,
	Andy Lutomirski <luto@amacapital.net>,
	Ingo Molnar <mingo@kernel.org>,
	Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	"linux-alpha@vger.kernel.org"
	<linux-arm-kernel@lists.infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	James Morse <james.morse@arm.com>,
	valentin.schneider@arm.com, Brian Gerst <brgerst@gmail.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Andrew Lutomirski <luto@kernel.org>,
	Borislav Petkov <bp@alien8.de>,
	Denys Vlasenko <dvlasenk@redhat.com>
Subject: [RFC][PATCH] objtool: STAC/CLAC validation
Date: Fri, 22 Feb 2019 23:26:35 +0100	[thread overview]
Message-ID: <20190222222635.GK14054@worktop.programming.kicks-ass.net> (raw)
In-Reply-To: <alpine.DEB.2.21.1902221853330.1666@nanos.tec.linutronix.de>

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 {

  reply	other threads:[~2019-02-22 22:26 UTC|newest]

Thread overview: 83+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-15 13:58 [PATCH v3 0/4] uaccess: Add unsafe accessors for arm64 Julien Thierry
2019-01-15 13:58 ` [PATCH v3 1/4] arm64: uaccess: Cleanup get/put_user() Julien Thierry
2019-01-15 13:58 ` [PATCH v3 2/4] arm64: uaccess: Implement unsafe accessors Julien Thierry
2019-01-15 13:58 ` [PATCH v3 3/4] uaccess: Check no rescheduling function is called in unsafe region Julien Thierry
2019-01-30 16:58   ` Valentin Schneider
2019-02-04 13:27     ` Julien Thierry
2019-02-11 13:45   ` Ingo Molnar
2019-02-11 13:51     ` Peter Zijlstra
2019-02-12  9:15       ` Julien Thierry
2019-02-13  8:21         ` Ingo Molnar
2019-02-13 10:35         ` Peter Zijlstra
2019-02-13 10:50           ` Julien Thierry
2019-02-13 13:17             ` Peter Zijlstra
2019-02-13 13:20               ` Peter Zijlstra
2019-02-13 14:00               ` Will Deacon
2019-02-13 14:07                 ` Julien Thierry
2019-02-13 14:17                 ` Peter Zijlstra
2019-02-13 14:24                   ` Julien Thierry
2019-02-13 14:40                     ` Peter Zijlstra
2019-02-13 15:08                       ` Peter Zijlstra
2019-02-13 14:25                 ` Peter Zijlstra
2019-02-13 14:39                   ` Julien Thierry
2019-02-13 14:41                     ` Peter Zijlstra
2019-02-13 15:45                       ` Peter Zijlstra
2019-02-13 18:54                         ` Peter Zijlstra
     [not found]                         ` <D61C430D-4321-4114-AB85-671A3C7B8EAE@amacapital.net>
2019-02-13 22:21                           ` Peter Zijlstra
2019-02-13 22:49                             ` Andy Lutomirski
2019-02-14 10:14                               ` [PATCH] sched/x86: Save [ER]FLAGS on context switch Peter Zijlstra
2019-02-14 16:18                                 ` Brian Gerst
2019-02-14 19:34                                   ` Peter Zijlstra
2019-02-15 14:34                                     ` Brian Gerst
2019-02-15 17:18                                     ` Linus Torvalds
2019-02-15 17:40                                       ` Peter Zijlstra
2019-02-15 18:28                                         ` Andy Lutomirski
2019-02-15 23:34                                         ` Peter Zijlstra
2019-02-16  0:21                                           ` Linus Torvalds
2019-02-16 10:32                                             ` Peter Zijlstra
2019-02-16  4:06                                 ` hpa
2019-02-16 10:30                                   ` Peter Zijlstra
2019-02-18 22:30                                     ` H. Peter Anvin
2019-02-19  0:24                                       ` Linus Torvalds
2019-02-19  2:20                                         ` Andy Lutomirski
2019-02-19  2:46                                           ` H. Peter Anvin
2019-02-19  9:07                                             ` Julien Thierry
2019-02-19  8:53                                         ` Julien Thierry
2019-02-19  9:15                                         ` Peter Zijlstra
2019-02-19  9:19                                           ` Peter Zijlstra
2019-02-19  9:04                                       ` Peter Zijlstra
2019-02-19  9:21                                         ` hpa
2019-02-19  9:44                                         ` Peter Zijlstra
2019-02-19 11:38                                           ` Thomas Gleixner
2019-02-19 11:58                                             ` Peter Zijlstra
2019-02-19 12:48                                         ` Will Deacon
2019-02-20 22:55                                           ` H. Peter Anvin
2019-02-21 12:06                                             ` Julien Thierry
2019-02-21 21:35                                               ` Thomas Gleixner
2019-02-21 22:08                                                 ` Linus Torvalds
2019-02-22 12:58                                                   ` Peter Zijlstra
2019-02-22 18:10                                                   ` Thomas Gleixner
2019-02-22 22:26                                                     ` Peter Zijlstra [this message]
2019-02-22 23:34                                                       ` [RFC][PATCH] objtool: STAC/CLAC validation Linus Torvalds
2019-02-23  8:43                                                         ` Peter Zijlstra
2019-02-22 23:39                                                       ` hpa
2019-02-23  8:39                                                         ` Peter Zijlstra
2019-02-25  8:47                                                           ` hpa
2019-02-25 13:21                                                             ` Peter Zijlstra
2019-03-01 15:07                                                               ` Peter Zijlstra
2019-02-25  8:49                                                           ` hpa
2019-02-22 23:55                                                       ` Andy Lutomirski
2019-02-23  8:37                                                         ` Peter Zijlstra
2019-02-23 10:52                                                           ` Peter Zijlstra
2019-02-25 10:51                                                         ` Peter Zijlstra
2019-02-25 11:53                                                           ` Peter Zijlstra
2019-02-25 15:36                                                             ` Andy Lutomirski
2019-02-25  8:33                                                       ` Julien Thierry
2019-02-25 11:55                                                         ` Peter Zijlstra
2019-02-21 12:46                                             ` [PATCH] sched/x86: Save [ER]FLAGS on context switch Will Deacon
2019-02-21 22:06                                               ` Andy Lutomirski
2019-02-18  9:03                                 ` [PATCH v2] " Peter Zijlstra
2019-02-13 23:19                         ` [PATCH v3 3/4] uaccess: Check no rescheduling function is called in unsafe region Linus Torvalds
2019-01-15 13:58 ` [PATCH v3 4/4] arm64: uaccess: Implement user_access_region_active Julien Thierry
2019-01-25 14:27 ` [PATCH v3 0/4] uaccess: Add unsafe accessors for arm64 Catalin Marinas
2019-01-30 16:17 ` Julien Thierry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190222222635.GK14054@worktop.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=bp@alien8.de \
    --cc=brgerst@gmail.com \
    --cc=catalin.marinas@arm.com \
    --cc=dvlasenk@redhat.com \
    --cc=hpa@zytor.com \
    --cc=james.morse@arm.com \
    --cc=jpoimboe@redhat.com \
    --cc=julien.thierry@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=luto@kernel.org \
    --cc=mingo@kernel.org \
    --cc=mingo@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=valentin.schneider@arm.com \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).