[PATCH 0/6] objtool: UACCESS validation

[PATCH 0/6] objtool: UACCESS validation

[Peter Zijlstra]

Teach objtool to validate the UACCESS (SMAP, PAN) rules which are currently
unenforced and (therefore obviously) violated.

UACCESS sections should be small; we want to limit the amount of code that can
touch userspace. Furthermore, UACCESS state isn't scheduled, this means that
anything that directly calls into the scheduler will result in random code
running with UACCESS enabled and possibly getting back into the UACCESS region
with UACCESS disabled and causing faults.

Forbid any CALL/RET while UACCESS is enabled; but provide an annotation to mark
(a very limited) set of functions as UACCESS-safe (eg. the planned:
unsafe_copy_{to,from}_user()).

---
 arch/x86/ia32/ia32_signal.c     |  29 ++++---
 arch/x86/include/asm/uaccess.h  |   4 +-
 include/linux/frame.h           |  49 ++++++++++-
 tools/objtool/Makefile          |   2 +-
 tools/objtool/arch.h            |   6 +-
 tools/objtool/arch/x86/decode.c |  22 ++++-
 tools/objtool/check.c           | 180 ++++++++++++++++++++++++++++++----------
 tools/objtool/check.h           |   3 +-
 tools/objtool/elf.h             |   1 +
 9 files changed, 234 insertions(+), 62 deletions(-)

[PATCH 1/6] x86/uaccess: Dont evaluate argument inside AC region

[Peter Zijlstra]

AC regions are special, make sure we don't do possibly complex
evaluations there.

Probably-Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/uaccess.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- 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();					\

[PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak

[Peter Zijlstra]

Don't call load_gs_index() with AC set; delay the segment setting
until after the AC section.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/ia32/ia32_signal.c |   29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

--- 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(struc
 		 * 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(struc
 		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();

[PATCH 3/6] objtool: Set insn->func for alternatives

[Peter Zijlstra]

Make sure we set the function association for alternative function
sequences; they are after all still part of the function.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |    1 +
 1 file changed, 1 insertion(+)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -695,6 +695,7 @@ static int handle_group_alt(struct objto
 		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)

[PATCH 4/6] objtool: Replace STACK_FRAME_NON_STANDARD annotation

[Peter Zijlstra]

Replace the existing STACK_FRAME_NON_STANDARD annotation with a
'better' scheme.

The old annotation works by taking the address of a function; this
is visible to the compiler and might affect code generation (the
function pointer escapes). The new annotation simply stores the
function name and has objtool do a symtab lookup.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/frame.h |   19 +++++++++++++---
 tools/objtool/check.c |   58 +++++++++++++++++++++-----------------------------
 tools/objtool/check.h |    1 
 3 files changed, 41 insertions(+), 37 deletions(-)

--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -11,9 +11,22 @@
  *
  * For more information, see tools/objtool/Documentation/stack-validation.txt.
  */
-#define STACK_FRAME_NON_STANDARD(func) \
-	static void __used __section(.discard.func_stack_frame_non_standard) \
-		*__func_stack_frame_non_standard_##func = func
+#define STACK_FRAME_NON_STANDARD(func)					\
+	asm (".pushsection .discard.nonstd_frame_strtab, \"S\", @3\n\t"	\
+	     "999: .ascii \"" #func "\"\n\t"				\
+	     "     .byte 0\n\t"						\
+	     ".popsection\n\t"						\
+	     ".pushsection .discard.nonstd_frame\n\t"			\
+	     ".long 999b - .\n\t"					\
+	     ".popsection\n\t")
+
+/*
+ * SHT_STRTAB(@3) sections should start with a \0 byte such that the 0 offset
+ * encodes the NULL string.
+ */
+asm (".pushsection .discard.nonstd_frame_strtab, \"S\", @3\n\t"
+     ".byte 0\n\t"
+     ".popsection\n\t");
 
 #else /* !CONFIG_STACK_VALIDATION */
 
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -105,29 +105,6 @@ static struct instruction *next_insn_sam
 	     insn = next_insn_same_sec(file, insn))
 
 /*
- * Check if the function has been manually whitelisted with the
- * STACK_FRAME_NON_STANDARD macro, or if it should be automatically whitelisted
- * due to its use of a context switching instruction.
- */
-static bool ignore_func(struct objtool_file *file, struct symbol *func)
-{
-	struct rela *rela;
-
-	/* check for STACK_FRAME_NON_STANDARD */
-	if (file->whitelist && file->whitelist->rela)
-		list_for_each_entry(rela, &file->whitelist->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.
  *
  * For global functions which are outside the scope of this object file, we
@@ -434,21 +411,37 @@ static int add_dead_ends(struct objtool_
 static void add_ignores(struct objtool_file *file)
 {
 	struct instruction *insn;
-	struct section *sec;
+	struct section *strtab_sec, *sec;
 	struct symbol *func;
+	struct rela *rela;
+	const char *name;
 
-	for_each_sec(file, sec) {
-		list_for_each_entry(func, &sec->symbol_list, list) {
-			if (func->type != STT_FUNC)
-				continue;
+	sec = find_section_by_name(file->elf, ".rela.discard.nonstd_frame");
+	if (!sec)
+		return;
 
-			if (!ignore_func(file, func))
-				continue;
+	strtab_sec = find_section_by_name(file->elf, ".discard.nonstd_frame_strtab");
+	if (!strtab_sec) {
+		WARN("missing nonstd_frame strtab");
+		return;
+	}
 
-			func_for_each_insn_all(file, func, insn)
-				insn->ignore = true;
+	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;
 		}
+
+		name = elf_strptr(file->elf->elf, strtab_sec->idx, rela->addend);
+		func = find_symbol_by_name(file->elf, name);
+		if (!func)
+			continue;
+
+		func_for_each_insn_all(file, func, insn)
+			insn->ignore = true;
 	}
+
+	return;
 }
 
 /*
@@ -2198,7 +2191,6 @@ 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.c_file = find_section_by_name(file.elf, ".comment");
 	file.ignore_unreachables = no_unreachable;
 	file.hints = false;
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -60,7 +60,6 @@ struct objtool_file {
 	struct elf *elf;
 	struct list_head insn_list;
 	DECLARE_HASHTABLE(insn_hash, 16);
-	struct section *whitelist;
 	bool ignore_unreachables, c_file, hints, rodata;
 };
 

[PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

It is important that UACCESS regions are as small as possible;
furthermore the UACCESS state is not scheduled, so doing anything that
might directly call into the scheduler will cause random code to be
ran with UACCESS enabled.

Teach objtool too track UACCESS state and warn about any CALL made
while UACCESS is enabled. This very much includes the __fentry__()
tracing calls and __preempt_schedule() calls.

Note that exceptions _do_ save/restore the UACCESS state, and therefore
they can drive preemption. This also means that all exception handlers
must have an otherwise dedundant UACCESS disable instruction;
therefore ignore this warning for !STT_FUNC code (exception handlers
are not normal functions).

It also provides a UACCESS_SAFE() annotation which allows explicit
annotation. This is meant to be used for future things like:
unsafe_copy_{to,from}_user().

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/frame.h           |   30 +++++++++++-
 tools/objtool/arch.h            |    4 +
 tools/objtool/arch/x86/decode.c |   14 +++++
 tools/objtool/check.c           |  100 ++++++++++++++++++++++++++++++++++++----
 tools/objtool/check.h           |    2 
 tools/objtool/elf.h             |    1 
 6 files changed, 138 insertions(+), 13 deletions(-)

--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -28,10 +28,38 @@ asm (".pushsection .discard.nonstd_frame
      ".byte 0\n\t"
      ".popsection\n\t");
 
+/*
+ * This macro marks functions as UACCESS-safe, that is, it is safe to call from an
+ * UACCESS enabled region (typically user_access_begin() /
+ * user_access_end()).
+ *
+ * These functions in turn will only call UACCESS-safe functions themselves (which
+ * precludes tracing, including __fentry__ and scheduling, including
+ * preempt_enable).
+ *
+ * UACCESS-safe functions will obviously also not change UACCESS themselves.
+ */
+#define UACCESS_SAFE(func)						\
+	asm (".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t"	\
+	     "999: .ascii \"" #func "\"\n\t"				\
+	     "     .byte 0\n\t"						\
+	     ".popsection\n\t"						\
+	     ".pushsection .discard.uaccess_safe\n\t"			\
+	     ".long 999b - .\n\t"					\
+	     ".popsection")
+
+/*
+ * SHT_STRTAB(@3) sections should start with a \0 byte such that the 0 offset
+ * encodes the NULL string.
+ */
+asm(".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t"
+    ".byte 0\n\t"
+    ".popsection\n\t");
+
 #else /* !CONFIG_STACK_VALIDATION */
 
 #define STACK_FRAME_NON_STANDARD(func)
+#define AC_SAFE(func)
 
 #endif /* CONFIG_STACK_VALIDATION */
-
 #endif /* _LINUX_FRAME_H */
--- 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 {
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *
 
 	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;
 
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -444,6 +444,40 @@ static void add_ignores(struct objtool_f
 	return;
 }
 
+static void add_uaccess_safe(struct objtool_file *file)
+{
+	struct section *strtab_sec, *sec;
+	struct symbol *func;
+	struct rela *rela;
+	const char *name;
+
+	sec = find_section_by_name(file->elf, ".rela.discard.uaccess_safe");
+	if (!sec)
+		return;
+
+	strtab_sec = find_section_by_name(file->elf, ".discard.uaccess_safe_strtab");
+	if (!strtab_sec) {
+		WARN("missing uaccess_safe strtab");
+		return;
+	}
+
+	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;
+		}
+
+		name = elf_strptr(file->elf->elf, strtab_sec->idx, rela->addend);
+		func = find_symbol_by_name(file->elf, name);
+		if (!func)
+			continue;
+
+		func->uaccess_safe = true;
+	}
+
+	return;
+}
+
 /*
  * 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.
@@ -1232,6 +1266,7 @@ static int decode_sections(struct objtoo
 		return ret;
 
 	add_ignores(file);
+	add_uaccess_safe(file);
 
 	ret = add_nospec_ignores(file);
 	if (ret)
@@ -1792,6 +1827,22 @@ static bool insn_state_match(struct inst
 	return false;
 }
 
+static inline bool insn_dest_uaccess_safe(struct instruction *insn)
+{
+	if (!insn->call_dest)
+		return false;
+
+	return insn->call_dest->uaccess_safe;
+}
+
+static inline const char *insn_dest_name(struct instruction *insn)
+{
+	if (!insn->call_dest)
+		return "{dynamic}";
+
+	return insn->call_dest->name;
+}
+
 /*
  * Follow the branch starting at the given instruction, and recursively follow
  * any other branches (jumps).  Meanwhile, track the frame pointer state at
@@ -1896,6 +1947,9 @@ static int validate_branch(struct objtoo
 		switch (insn->type) {
 
 		case INSN_RETURN:
+			if (state.uaccess)
+				WARN_FUNC("return with UACCESS enabled", sec, insn->offset);
+
 			if (func && has_modified_stack_frame(&state)) {
 				WARN_FUNC("return with modified stack frame",
 					  sec, insn->offset);
@@ -1911,17 +1965,24 @@ static int validate_branch(struct objtoo
 			return 0;
 
 		case INSN_CALL:
-			if (is_fentry_call(insn))
-				break;
+		case INSN_CALL_DYNAMIC:
+			if ((state.uaccess_safe || state.uaccess) &&
+			    !insn_dest_uaccess_safe(insn)) {
+				WARN_FUNC("call to %s() with UACCESS enabled",
+					  sec, insn->offset, insn_dest_name(insn));
+			}
 
-			ret = dead_end_function(file, insn->call_dest);
-			if (ret == 1)
-				return 0;
-			if (ret == -1)
-				return 1;
+			if (insn->type == INSN_CALL) {
+				if (is_fentry_call(insn))
+					break;
+
+				ret = dead_end_function(file, insn->call_dest);
+				if (ret == 1)
+					return 0;
+				if (ret == -1)
+					return 1;
+			}
 
-			/* fallthrough */
-		case INSN_CALL_DYNAMIC:
 			if (!no_fp && func && !has_valid_stack_frame(&state)) {
 				WARN_FUNC("call without frame pointer save/setup",
 					  sec, insn->offset);
@@ -1974,6 +2035,25 @@ static int validate_branch(struct objtoo
 
 			break;
 
+		case INSN_STAC:
+			if (state.uaccess_safe || state.uaccess)
+				WARN_FUNC("recursive UACCESS enable", sec, insn->offset);
+
+			state.uaccess = true;
+			break;
+
+		case INSN_CLAC:
+			if (!state.uaccess && insn->func)
+				WARN_FUNC("redundant UACCESS disable", sec, insn->offset);
+
+			if (state.uaccess_safe) {
+				WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
+				break;
+			}
+
+			state.uaccess = false;
+			break;
+
 		default:
 			break;
 		}
@@ -2135,6 +2215,8 @@ static int validate_functions(struct obj
 			if (!insn || insn->ignore)
 				continue;
 
+			state.uaccess_safe = func->uaccess_safe;
+
 			ret = validate_branch(file, insn, state);
 			warnings += ret;
 		}
--- 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, uaccess, uaccess_safe;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];
 };
--- 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 uaccess_safe;
 };
 
 struct rela {

[PATCH 6/6] objtool: Add Direction Flag validation

[Peter Zijlstra]

Having DF escape is BAD(tm).

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/arch.h            |    4 +++-
 tools/objtool/arch/x86/decode.c |    8 ++++++++
 tools/objtool/check.c           |   21 +++++++++++++++++++++
 tools/objtool/check.h           |    2 +-
 4 files changed, 33 insertions(+), 2 deletions(-)

--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -35,7 +35,9 @@
 #define INSN_NOP		10
 #define INSN_STAC		11
 #define INSN_CLAC		12
-#define INSN_OTHER		13
+#define INSN_STD		13
+#define INSN_CLD		14
+#define INSN_OTHER		15
 #define INSN_LAST		INSN_OTHER
 
 enum op_dest_type {
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -456,6 +456,14 @@ int arch_decode_instruction(struct elf *
 		*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)
 
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1950,6 +1950,9 @@ static int validate_branch(struct objtoo
 			if (state.uaccess)
 				WARN_FUNC("return with UACCESS enabled", sec, insn->offset);
 
+			if (state.df)
+				WARN_FUNC("return with DF set", sec, insn->offset);
+
 			if (func && has_modified_stack_frame(&state)) {
 				WARN_FUNC("return with modified stack frame",
 					  sec, insn->offset);
@@ -1971,6 +1974,10 @@ static int validate_branch(struct objtoo
 				WARN_FUNC("call to %s() with UACCESS enabled",
 					  sec, insn->offset, insn_dest_name(insn));
 			}
+			if (state.df) {
+				WARN_FUNC("call to %s() with DF set",
+					  sec, insn->offset, insn_dest_name(insn));
+			}
 
 			if (insn->type == INSN_CALL) {
 				if (is_fentry_call(insn))
@@ -2054,6 +2061,20 @@ static int validate_branch(struct objtoo
 			state.uaccess = false;
 			break;
 
+		case INSN_STD:
+			if (state.df)
+				WARN_FUNC("recursive STD", sec, insn->offset);
+
+			state.df = true;
+			break;
+
+		case INSN_CLD:
+			if (!state.df && insn->func)
+				WARN_FUNC("redundant CLD", sec, insn->offset);
+
+			state.df = false;
+			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;
+	bool drap, end, uaccess, uaccess_safe, df;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];
 };

Re: [PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak

[Andy Lutomirski]

On Mon, Feb 25, 2019 at 4:53 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Don't call load_gs_index() with AC set; delay the segment setting
> until after the AC section.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/ia32/ia32_signal.c |   29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> --- 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);           \
>  }

This is so tangled.

How about changing RELOAD_SEG to replace unsigned int pre =
GET_SEG(seg); with unsigned int pre = (seg); to make it less magic.
Then do:

unsigned int gs = GET_SEG(gs);

...

RELOAD_SEG(gs);

And now the code actually does what it looks like it does.

Re: [PATCH 1/6] x86/uaccess: Dont evaluate argument inside AC region

[Andy Lutomirski]

On Mon, Feb 25, 2019 at 4:53 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> AC regions are special, make sure we don't do possibly complex
> evaluations there.
>
> Probably-Signed-off-by: Andy Lutomirski <luto@kernel.org>

I put a slightly fancier version here last night so that the 0day bot
could chew on it:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/fixes&id=b294da548d92320f194172272dda1f734e51013d

want to snarf it up?  It's the same thing except that I added Linus'
suggested change, found the offending patch that broke it (hi Linus!)
and added a bit more changelog :)

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Andy Lutomirski]

On Mon, Feb 25, 2019 at 4:53 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> It is important that UACCESS regions are as small as possible;
> furthermore the UACCESS state is not scheduled, so doing anything that
> might directly call into the scheduler will cause random code to be
> ran with UACCESS enabled.
>
> Teach objtool too track UACCESS state and warn about any CALL made
> while UACCESS is enabled. This very much includes the __fentry__()
> tracing calls and __preempt_schedule() calls.
>
> Note that exceptions _do_ save/restore the UACCESS state, and therefore
> they can drive preemption. This also means that all exception handlers
> must have an otherwise dedundant UACCESS disable instruction;
> therefore ignore this warning for !STT_FUNC code (exception handlers
> are not normal functions).
>
> It also provides a UACCESS_SAFE() annotation which allows explicit
> annotation. This is meant to be used for future things like:
> unsafe_copy_{to,from}_user().
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/frame.h           |   30 +++++++++++-
>  tools/objtool/arch.h            |    4 +
>  tools/objtool/arch/x86/decode.c |   14 +++++
>  tools/objtool/check.c           |  100 ++++++++++++++++++++++++++++++++++++----
>  tools/objtool/check.h           |    2
>  tools/objtool/elf.h             |    1
>  6 files changed, 138 insertions(+), 13 deletions(-)
>
> --- a/include/linux/frame.h
> +++ b/include/linux/frame.h
> @@ -28,10 +28,38 @@ asm (".pushsection .discard.nonstd_frame
>       ".byte 0\n\t"
>       ".popsection\n\t");
>
> +/*
> + * This macro marks functions as UACCESS-safe, that is, it is safe to call from an
> + * UACCESS enabled region (typically user_access_begin() /
> + * user_access_end()).
> + *
> + * These functions in turn will only call UACCESS-safe functions themselves (which
> + * precludes tracing, including __fentry__ and scheduling, including
> + * preempt_enable).
> + *
> + * UACCESS-safe functions will obviously also not change UACCESS themselves.
> + */
> +#define UACCESS_SAFE(func)                                             \
> +       asm (".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t" \
> +            "999: .ascii \"" #func "\"\n\t"                            \
> +            "     .byte 0\n\t"                                         \
> +            ".popsection\n\t"                                          \
> +            ".pushsection .discard.uaccess_safe\n\t"                   \
> +            ".long 999b - .\n\t"                                       \
> +            ".popsection")

Minor nit: using big numbers like 999: like this always bugs me.  It
relies on there not being a macro nested inside or outside this that
uses the same number.  My general preference is to do something like
.Ldescription_\@ instead.

Otherwise this looks conceptually good :)

Re: [PATCH 1/6] x86/uaccess: Dont evaluate argument inside AC region

[Peter Zijlstra]

On Mon, Feb 25, 2019 at 07:43:53AM -0800, Andy Lutomirski wrote:
> On Mon, Feb 25, 2019 at 4:53 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > AC regions are special, make sure we don't do possibly complex
> > evaluations there.
> >
> > Probably-Signed-off-by: Andy Lutomirski <luto@kernel.org>
> 
> I put a slightly fancier version here last night so that the 0day bot
> could chew on it:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/fixes&id=b294da548d92320f194172272dda1f734e51013d
> 
> want to snarf it up?  It's the same thing except that I added Linus'
> suggested change, found the offending patch that broke it (hi Linus!)
> and added a bit more changelog :)

Sure; I'll take that one.

Re: [PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak

[Peter Zijlstra]

On Mon, Feb 25, 2019 at 07:41:50AM -0800, Andy Lutomirski wrote:
> This is so tangled.
> 
> How about changing RELOAD_SEG to replace unsigned int pre =
> GET_SEG(seg); with unsigned int pre = (seg); to make it less magic.
> Then do:
> 
> unsigned int gs = GET_SEG(gs);
> 
> ...
> 
> RELOAD_SEG(gs);
> 
> And now the code actually does what it looks like it does.

Is this what you mean?

---
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 321fe5f5d0e9..e04eeeddcc35 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -53,17 +53,16 @@
 #define GET_SEG(seg)		({			\
 	unsigned short tmp;				\
 	get_user_ex(tmp, &sc->seg);			\
-	tmp;						\
+	tmp | 3;					\
 })
 
 #define COPY_SEG_CPL3(seg)	do {			\
-	regs->seg = GET_SEG(seg) | 3;			\
+	regs->seg = GET_SEG(seg);			\
 } while (0)
 
 #define RELOAD_SEG(seg)		{		\
-	unsigned int pre = GET_SEG(seg);	\
+	unsigned int pre = (seg);		\
 	unsigned int cur = get_user_seg(seg);	\
-	pre |= 3;				\
 	if (pre != cur)				\
 		set_user_seg(seg, pre);		\
 }
@@ -72,6 +71,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 				   struct sigcontext_32 __user *sc)
 {
 	unsigned int tmpflags, err = 0;
+	u16 gs, fs, es, ds;
 	void __user *buf;
 	u32 tmp;
 
@@ -79,16 +79,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 	current->restart_block.fn = do_no_restart_syscall;
 
 	get_user_try {
-		/*
-		 * Reload fs and gs if they have changed in the signal
-		 * handler.  This does not handle long fs/gs base changes in
-		 * 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);
+		gs = GET_SEG(gs);
+		fs = GET_SEG(fs);
+		ds = GET_SEG(ds);
+		es = GET_SEG(es);
 
 		COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
 		COPY(dx); COPY(cx); COPY(ip); COPY(ax);
@@ -106,6 +100,17 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 		buf = compat_ptr(tmp);
 	} get_user_catch(err);
 
+	/*
+	 * Reload fs and gs if they have changed in the signal
+	 * handler.  This does not handle long fs/gs base changes in
+	 * 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);
+
 	err |= fpu__restore_sig(buf, 1);
 
 	force_iret();

Re: [PATCH 4/6] objtool: Replace STACK_FRAME_NON_STANDARD annotation

[Josh Poimboeuf]

On Mon, Feb 25, 2019 at 01:43:34PM +0100, Peter Zijlstra wrote:
> Replace the existing STACK_FRAME_NON_STANDARD annotation with a
> 'better' scheme.
> 
> The old annotation works by taking the address of a function; this
> is visible to the compiler and might affect code generation (the
> function pointer escapes). The new annotation simply stores the
> function name and has objtool do a symtab lookup.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/frame.h |   19 +++++++++++++---
>  tools/objtool/check.c |   58 +++++++++++++++++++++-----------------------------
>  tools/objtool/check.h |    1 
>  3 files changed, 41 insertions(+), 37 deletions(-)
> 
> --- a/include/linux/frame.h
> +++ b/include/linux/frame.h
> @@ -11,9 +11,22 @@
>   *
>   * For more information, see tools/objtool/Documentation/stack-validation.txt.
>   */
> -#define STACK_FRAME_NON_STANDARD(func) \
> -	static void __used __section(.discard.func_stack_frame_non_standard) \
> -		*__func_stack_frame_non_standard_##func = func
> +#define STACK_FRAME_NON_STANDARD(func)					\
> +	asm (".pushsection .discard.nonstd_frame_strtab, \"S\", @3\n\t"	\
> +	     "999: .ascii \"" #func "\"\n\t"				\
> +	     "     .byte 0\n\t"						\
> +	     ".popsection\n\t"						\
> +	     ".pushsection .discard.nonstd_frame\n\t"			\
> +	     ".long 999b - .\n\t"					\
> +	     ".popsection\n\t")
> +

I don't think this will work in the case where GCC does an IPA
optimization and renames the function (e.g., renames func to
func.isra.1234), right?  That might be a deal breaker...

> +/*
> + * SHT_STRTAB(@3) sections should start with a \0 byte such that the 0 offset
> + * encodes the NULL string.
> + */
> +asm (".pushsection .discard.nonstd_frame_strtab, \"S\", @3\n\t"
> +     ".byte 0\n\t"
> +     ".popsection\n\t");

Including the file creates the section, which is a bit nasty.  Instead
just change STACK_FRAME_NON_STANDARD to prefix every string with a \0?

-- 
Josh

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

On Mon, Feb 25, 2019 at 07:53:01AM -0800, Andy Lutomirski wrote:
> On Mon, Feb 25, 2019 at 4:53 AM Peter Zijlstra <peterz@infradead.org> wrote:

> > +#define UACCESS_SAFE(func)                                             \
> > +       asm (".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t" \
> > +            "999: .ascii \"" #func "\"\n\t"                            \
> > +            "     .byte 0\n\t"                                         \
> > +            ".popsection\n\t"                                          \
> > +            ".pushsection .discard.uaccess_safe\n\t"                   \
> > +            ".long 999b - .\n\t"                                       \
> > +            ".popsection")
> 
> Minor nit: using big numbers like 999: like this always bugs me.  It
> relies on there not being a macro nested inside or outside this that
> uses the same number.  My general preference is to do something like
> .Ldescription_\@ instead.
> 
> Otherwise this looks conceptually good :)

I seem to remember here being an issue with the \@ thing. Notably we're
not using it in nospsec-branch.h.

I'll see if I can dig up why we decided not to use it there.

Re: [PATCH 4/6] objtool: Replace STACK_FRAME_NON_STANDARD annotation

[Peter Zijlstra]

On Mon, Feb 25, 2019 at 10:11:24AM -0600, Josh Poimboeuf wrote:
> On Mon, Feb 25, 2019 at 01:43:34PM +0100, Peter Zijlstra wrote:
> > Replace the existing STACK_FRAME_NON_STANDARD annotation with a
> > 'better' scheme.
> > 
> > The old annotation works by taking the address of a function; this
> > is visible to the compiler and might affect code generation (the
> > function pointer escapes). The new annotation simply stores the
> > function name and has objtool do a symtab lookup.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  include/linux/frame.h |   19 +++++++++++++---
> >  tools/objtool/check.c |   58 +++++++++++++++++++++-----------------------------
> >  tools/objtool/check.h |    1 
> >  3 files changed, 41 insertions(+), 37 deletions(-)
> > 
> > --- a/include/linux/frame.h
> > +++ b/include/linux/frame.h
> > @@ -11,9 +11,22 @@
> >   *
> >   * For more information, see tools/objtool/Documentation/stack-validation.txt.
> >   */
> > -#define STACK_FRAME_NON_STANDARD(func) \
> > -	static void __used __section(.discard.func_stack_frame_non_standard) \
> > -		*__func_stack_frame_non_standard_##func = func
> > +#define STACK_FRAME_NON_STANDARD(func)					\
> > +	asm (".pushsection .discard.nonstd_frame_strtab, \"S\", @3\n\t"	\
> > +	     "999: .ascii \"" #func "\"\n\t"				\
> > +	     "     .byte 0\n\t"						\
> > +	     ".popsection\n\t"						\
> > +	     ".pushsection .discard.nonstd_frame\n\t"			\
> > +	     ".long 999b - .\n\t"					\
> > +	     ".popsection\n\t")
> > +
> 
> I don't think this will work in the case where GCC does an IPA
> optimization and renames the function (e.g., renames func to
> func.isra.1234), right?  That might be a deal breaker...

*groan*... indeed :/ So back to the old scheme then? Andy, any other
clever ideas?

> > +/*
> > + * SHT_STRTAB(@3) sections should start with a \0 byte such that the 0 offset
> > + * encodes the NULL string.
> > + */
> > +asm (".pushsection .discard.nonstd_frame_strtab, \"S\", @3\n\t"
> > +     ".byte 0\n\t"
> > +     ".popsection\n\t");
> 
> Including the file creates the section, which is a bit nasty.  Instead
> just change STACK_FRAME_NON_STANDARD to prefix every string with a \0?

That would get us \0\0 between every string, which I suppose is in-spec,
but weird. I figured that since it is a .discard. section, getting it
wasn't a problem.

Re: [PATCH 4/6] objtool: Replace STACK_FRAME_NON_STANDARD annotation

[Josh Poimboeuf]

On Mon, Feb 25, 2019 at 05:17:22PM +0100, Peter Zijlstra wrote:
> > Including the file creates the section, which is a bit nasty.  Instead
> > just change STACK_FRAME_NON_STANDARD to prefix every string with a \0?
> 
> That would get us \0\0 between every string, which I suppose is in-spec,
> but weird. I figured that since it is a .discard. section, getting it
> wasn't a problem.

It would just bother me, as a power user of readelf, to see those unused
sections everywhere :-)  Maybe it's not a big deal though.

-- 
Josh

Re: [PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak

[Andy Lutomirski]

> On Feb 25, 2019, at 8:10 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> On Mon, Feb 25, 2019 at 07:41:50AM -0800, Andy Lutomirski wrote:
>> This is so tangled.
>> 
>> How about changing RELOAD_SEG to replace unsigned int pre =
>> GET_SEG(seg); with unsigned int pre = (seg); to make it less magic.
>> Then do:
>> 
>> unsigned int gs = GET_SEG(gs);
>> 
>> ...
>> 
>> RELOAD_SEG(gs);
>> 
>> And now the code actually does what it looks like it does.
> 
> Is this what you mean?

Yes, except:

> 
> ---
> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
> index 321fe5f5d0e9..e04eeeddcc35 100644
> --- a/arch/x86/ia32/ia32_signal.c
> +++ b/arch/x86/ia32/ia32_signal.c
> @@ -53,17 +53,16 @@
> #define GET_SEG(seg)        ({            \
>    unsigned short tmp;                \
>    get_user_ex(tmp, &sc->seg);            \
> -    tmp;                        \
> +    tmp | 3;                    \
> })
> 

Drop this part.

> #define COPY_SEG_CPL3(seg)    do {            \
> -    regs->seg = GET_SEG(seg) | 3;            \
> +    regs->seg = GET_SEG(seg);            \
> } while (0)

And this.

Unfortunately, whether we want the | 3 varies by segment. For FS and GS, we definitely don’t want it, since 0 is a common and important value, and 3 is a deeply screwed up value.  (3 is legal to *write* to GS, and it sticks, but IRET silently changes it to 0, because the original 386 designers (I assume) confused themselves.

> 
> #define RELOAD_SEG(seg)        {        \
> -    unsigned int pre = GET_SEG(seg);    \
> +    unsigned int pre = (seg);        \
>    unsigned int cur = get_user_seg(seg);    \
> -    pre |= 3;                \
>    if (pre != cur)                \
>        set_user_seg(seg, pre);        \
> }
> @@ -72,6 +71,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>                   struct sigcontext_32 __user *sc)
> {
>    unsigned int tmpflags, err = 0;
> +    u16 gs, fs, es, ds;
>    void __user *buf;
>    u32 tmp;
> 
> @@ -79,16 +79,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>    current->restart_block.fn = do_no_restart_syscall;
> 
>    get_user_try {
> -        /*
> -         * Reload fs and gs if they have changed in the signal
> -         * handler.  This does not handle long fs/gs base changes in
> -         * 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);
> +        gs = GET_SEG(gs);
> +        fs = GET_SEG(fs);
> +        ds = GET_SEG(ds);
> +        es = GET_SEG(es);
> 
>        COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
>        COPY(dx); COPY(cx); COPY(ip); COPY(ax);
> @@ -106,6 +100,17 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>        buf = compat_ptr(tmp);
>    } get_user_catch(err);
> 
> +    /*
> +     * Reload fs and gs if they have changed in the signal
> +     * handler.  This does not handle long fs/gs base changes in
> +     * 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);
> +
>    err |= fpu__restore_sig(buf, 1);
> 
>    force_iret();

I

Re: [PATCH 1/6] x86/uaccess: Dont evaluate argument inside AC region

[Borislav Petkov]

On Mon, Feb 25, 2019 at 07:43:53AM -0800, Andy Lutomirski wrote:
> I put a slightly fancier version here last night so that the 0day bot
> could chew on it:

I'm guessing 0day bot didn't complain so far? Or should we give it one
more day?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak

[Peter Zijlstra]

On Mon, Feb 25, 2019 at 08:29:12AM -0800, Andy Lutomirski wrote:

> > diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
> > index 321fe5f5d0e9..e04eeeddcc35 100644
> > --- a/arch/x86/ia32/ia32_signal.c
> > +++ b/arch/x86/ia32/ia32_signal.c
> > @@ -53,17 +53,16 @@
> > #define GET_SEG(seg)        ({            \
> >    unsigned short tmp;                \
> >    get_user_ex(tmp, &sc->seg);            \
> > -    tmp;                        \
> > +    tmp | 3;                    \
> > })
> > 
> 
> Drop this part.
> 
> > #define COPY_SEG_CPL3(seg)    do {            \
> > -    regs->seg = GET_SEG(seg) | 3;            \
> > +    regs->seg = GET_SEG(seg);            \
> > } while (0)
> 
> And this.
> 
> Unfortunately, whether we want the | 3 varies by segment. For FS and
> GS, we definitely don’t want it, since 0 is a common and important
> value, and 3 is a deeply screwed up value.  (3 is legal to *write* to
> GS, and it sticks, but IRET silently changes it to 0, because the
> original 386 designers (I assume) confused themselves.

> > 
> > #define RELOAD_SEG(seg)        {        \
> > -    unsigned int pre = GET_SEG(seg);    \
> > +    unsigned int pre = (seg);        \
> >    unsigned int cur = get_user_seg(seg);    \
> > -    pre |= 3;                \

And here ?

> >    if (pre != cur)                \
> >        set_user_seg(seg, pre);        \
> > }

Thing is; afaict the current code _always_ does |3 on any GET_SET()
result.

If you want to change that; I'm fine with that, but lets not do that in
this same patch.

So then?

diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 321fe5f5d0e9..4d5fcd47ab75 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -61,9 +61,8 @@
 } while (0)
 
 #define RELOAD_SEG(seg)		{		\
-	unsigned int pre = GET_SEG(seg);	\
+	unsigned int pre = (seg) | 3;		\
 	unsigned int cur = get_user_seg(seg);	\
-	pre |= 3;				\
 	if (pre != cur)				\
 		set_user_seg(seg, pre);		\
 }
@@ -72,6 +71,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 				   struct sigcontext_32 __user *sc)
 {
 	unsigned int tmpflags, err = 0;
+	u16 gs, fs, es, ds;
 	void __user *buf;
 	u32 tmp;
 
@@ -79,16 +79,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 	current->restart_block.fn = do_no_restart_syscall;
 
 	get_user_try {
-		/*
-		 * Reload fs and gs if they have changed in the signal
-		 * handler.  This does not handle long fs/gs base changes in
-		 * 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);
+		gs = GET_SEG(gs);
+		fs = GET_SEG(fs);
+		ds = GET_SEG(ds);
+		es = GET_SEG(es);
 
 		COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
 		COPY(dx); COPY(cx); COPY(ip); COPY(ax);
@@ -106,6 +100,17 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 		buf = compat_ptr(tmp);
 	} get_user_catch(err);
 
+	/*
+	 * Reload fs and gs if they have changed in the signal
+	 * handler.  This does not handle long fs/gs base changes in
+	 * 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);
+
 	err |= fpu__restore_sig(buf, 1);
 
 	force_iret();

Re: [PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak

[Peter Zijlstra]

On Mon, Feb 25, 2019 at 05:37:45PM +0100, Peter Zijlstra wrote:
> +	RELOAD_SEG(gs);
> +	RELOAD_SEG(fs);
> +	RELOAD_SEG(ds);
> +	RELOAD_SEG(es);

Also; is that the canonical order ? It bugs the hell out of me to not
have that alphabetically correct. Shall I correct that?

Re: [PATCH 2/6] x86/ia32: Fix ia32_restore_sigcontext AC leak

[Andy Lutomirski]

On Mon, Feb 25, 2019 at 8:37 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Feb 25, 2019 at 08:29:12AM -0800, Andy Lutomirski wrote:
>
> > > diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
> > > index 321fe5f5d0e9..e04eeeddcc35 100644
> > > --- a/arch/x86/ia32/ia32_signal.c
> > > +++ b/arch/x86/ia32/ia32_signal.c
> > > @@ -53,17 +53,16 @@
> > > #define GET_SEG(seg)        ({            \
> > >    unsigned short tmp;                \
> > >    get_user_ex(tmp, &sc->seg);            \
> > > -    tmp;                        \
> > > +    tmp | 3;                    \
> > > })
> > >
> >
> > Drop this part.
> >
> > > #define COPY_SEG_CPL3(seg)    do {            \
> > > -    regs->seg = GET_SEG(seg) | 3;            \
> > > +    regs->seg = GET_SEG(seg);            \
> > > } while (0)
> >
> > And this.
> >
> > Unfortunately, whether we want the | 3 varies by segment. For FS and
> > GS, we definitely don’t want it, since 0 is a common and important
> > value, and 3 is a deeply screwed up value.  (3 is legal to *write* to
> > GS, and it sticks, but IRET silently changes it to 0, because the
> > original 386 designers (I assume) confused themselves.
>
> > >
> > > #define RELOAD_SEG(seg)        {        \
> > > -    unsigned int pre = GET_SEG(seg);    \
> > > +    unsigned int pre = (seg);        \
> > >    unsigned int cur = get_user_seg(seg);    \
> > > -    pre |= 3;                \
>
> And here ?
>
> > >    if (pre != cur)                \
> > >        set_user_seg(seg, pre);        \
> > > }
>
> Thing is; afaict the current code _always_ does |3 on any GET_SET()
> result.
>
> If you want to change that; I'm fine with that, but lets not do that in
> this same patch.

Ugh, you're right.  I bet I can come up with some awful case involving
ptrace and sigreturn where this causes problems.

I have a patch series I need to dust off that deletes this entire
file.  It's this and its parents:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=execve&id=0618fe90d8224979f70b15d33dcae75a403592e5

>
> So then?
>
> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
> index 321fe5f5d0e9..4d5fcd47ab75 100644
> --- a/arch/x86/ia32/ia32_signal.c
> +++ b/arch/x86/ia32/ia32_signal.c
> @@ -61,9 +61,8 @@
>  } while (0)
>
>  #define RELOAD_SEG(seg)                {               \
> -       unsigned int pre = GET_SEG(seg);        \
> +       unsigned int pre = (seg) | 3;           \
>         unsigned int cur = get_user_seg(seg);   \
> -       pre |= 3;                               \
>         if (pre != cur)                         \
>                 set_user_seg(seg, pre);         \
>  }
> @@ -72,6 +71,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>                                    struct sigcontext_32 __user *sc)
>  {
>         unsigned int tmpflags, err = 0;
> +       u16 gs, fs, es, ds;
>         void __user *buf;
>         u32 tmp;
>
> @@ -79,16 +79,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>         current->restart_block.fn = do_no_restart_syscall;
>
>         get_user_try {
> -               /*
> -                * Reload fs and gs if they have changed in the signal
> -                * handler.  This does not handle long fs/gs base changes in
> -                * 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);
> +               gs = GET_SEG(gs);
> +               fs = GET_SEG(fs);
> +               ds = GET_SEG(ds);
> +               es = GET_SEG(es);
>
>                 COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
>                 COPY(dx); COPY(cx); COPY(ip); COPY(ax);
> @@ -106,6 +100,17 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
>                 buf = compat_ptr(tmp);
>         } get_user_catch(err);
>
> +       /*
> +        * Reload fs and gs if they have changed in the signal
> +        * handler.  This does not handle long fs/gs base changes in
> +        * 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);
> +
>         err |= fpu__restore_sig(buf, 1);
>
>         force_iret();

Looks good to me.  The order of the restores shouldn't matter at all.

Re: [PATCH 1/6] x86/uaccess: Dont evaluate argument inside AC region

[Andy Lutomirski]

On Mon, Feb 25, 2019 at 8:37 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Feb 25, 2019 at 07:43:53AM -0800, Andy Lutomirski wrote:
> > I put a slightly fancier version here last night so that the 0day bot
> > could chew on it:
>
> I'm guessing 0day bot didn't complain so far? Or should we give it one
> more day?
>

No complaints yet.  I'll holler if it complains.  This one patch could
plausibly be -stable or 5.0 material, I suppose.

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

On Mon, Feb 25, 2019 at 05:12:34PM +0100, Peter Zijlstra wrote:
> On Mon, Feb 25, 2019 at 07:53:01AM -0800, Andy Lutomirski wrote:
> > On Mon, Feb 25, 2019 at 4:53 AM Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > +#define UACCESS_SAFE(func)                                             \
> > > +       asm (".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t" \
> > > +            "999: .ascii \"" #func "\"\n\t"                            \
> > > +            "     .byte 0\n\t"                                         \
> > > +            ".popsection\n\t"                                          \
> > > +            ".pushsection .discard.uaccess_safe\n\t"                   \
> > > +            ".long 999b - .\n\t"                                       \
> > > +            ".popsection")
> > 
> > Minor nit: using big numbers like 999: like this always bugs me.  It
> > relies on there not being a macro nested inside or outside this that
> > uses the same number.  My general preference is to do something like
> > .Ldescription_\@ instead.
> > 
> > Otherwise this looks conceptually good :)
> 
> I seem to remember here being an issue with the \@ thing. Notably we're
> not using it in nospsec-branch.h.
> 
> I'll see if I can dig up why we decided not to use it there.

Ah, so the thing we need for inline-asm is %=, but Linus didn't like it:

  https://lkml.kernel.org/r/20180111175626.GJ6176@hirez.programming.kicks-ass.net

Also, I think at the time we didn't use it because backporting efforts.

I can't really find what happened between:

  https://lkml.kernel.org/r/1515508997-6154-2-git-send-email-dwmw@amazon.co.uk

and

  https://lkml.kernel.org/r/1515707194-20531-4-git-send-email-dwmw@amazon.co.uk

But the %= thing went away.

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Linus Torvalds]

On Mon, Feb 25, 2019 at 9:15 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Ah, so the thing we need for inline-asm is %=, but Linus didn't like it:

Well, to be fair, I also didn't like the random big numbers.

I think that inline asm that has local labels should just use simple
enumeration, and always use the direction suffix to make those local
labels be unambiguous.

Now, there are cases where we may not be able to do that, because we
have internal macros within the asm that _also_ wants to do its own
numbers. This isn't such a case.

For that case, you might actually want to use a fancy "get me a unique
number and prefix", but it should be seen as the exception rather than
the first solution, because the code starts looking really random.

             Linus

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Josh Poimboeuf]

On Mon, Feb 25, 2019 at 06:15:31PM +0100, Peter Zijlstra wrote:
> Also, I think at the time we didn't use it because backporting efforts.

This part may have been my fault.  I spread a vicious rumor (based on
reading docs instead of code) that old versions of GCC don't support
'%='.  But it's actually been around since GCC 2.8.

-- 
Josh

[tip:x86/urgent] x86/uaccess: Don't leak the AC flag into __put_user() value evaluation

[tip-bot for Andy Lutomirski]

Commit-ID:  1ee2bd5e09195d5476daefec5c64ba597a0a9920
Gitweb:     https://git.kernel.org/tip/1ee2bd5e09195d5476daefec5c64ba597a0a9920
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Fri, 22 Feb 2019 17:17:04 -0800
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Mon, 25 Feb 2019 18:55:04 +0100

x86/uaccess: Don't leak the AC flag into __put_user() value evaluation

When calling __put_user(foo(), ptr), the __put_user() macro would call
foo() in between __uaccess_begin() and __uaccess_end().  If that code
were buggy, then those bugs would be run without SMAP protection.

Fortunately, there seem to be few instances of the problem in the
kernel. Nevertheless, __put_user() should be fixed to avoid doing this.
Therefore, evaluate __put_user()'s argument before setting AC.

This issue was noticed when an objtool hack by Peter Zijlstra complained
about genregs_get() and I compared the assembly output to the C source.

 [ bp: Massage commit message. ]

Fixes: 11f1a4b9755f ("x86: reorganize SMAP handling in user space accesses")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/20190225125231.845656645@infradead.org
---
 arch/x86/include/asm/uaccess.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a77445d1b034..d7688efacf29 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -284,7 +284,7 @@ do {									\
 		__put_user_goto(x, ptr, "l", "k", "ir", label);		\
 		break;							\
 	case 8:								\
-		__put_user_goto_u64((__typeof__(*ptr))(x), ptr, label);	\
+		__put_user_goto_u64(x, ptr, label);	\
 		break;							\
 	default:							\
 		__put_user_bad();					\
@@ -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();					\

Re: [PATCH 1/6] x86/uaccess: Dont evaluate argument inside AC region

[Linus Torvalds]

On Mon, Feb 25, 2019 at 7:44 AM Andy Lutomirski <luto@amacapital.net> wrote:
>
> I put a slightly fancier version here last night so that the 0day bot
> could chew on it:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/fixes&id=b294da548d92320f194172272dda1f734e51013d

Ack. Even if it does seem to get the whitespace wrong and not align things ;)

             Linus

Re: [PATCH 1/6] x86/uaccess: Dont evaluate argument inside AC region

[Borislav Petkov]

On Mon, Feb 25, 2019 at 11:09:53AM -0800, Linus Torvalds wrote:
> Ack. Even if it does seem to get the whitespace wrong and not align
> things ;)

It is the top commit so fixed up and added your ACK. :)

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[tip:x86/urgent] x86/uaccess: Don't leak the AC flag into __put_user() value evaluation

[tip-bot for Andy Lutomirski]

Commit-ID:  2a418cf3f5f1caf911af288e978d61c9844b0695
Gitweb:     https://git.kernel.org/tip/2a418cf3f5f1caf911af288e978d61c9844b0695
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Fri, 22 Feb 2019 17:17:04 -0800
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Mon, 25 Feb 2019 20:17:05 +0100

x86/uaccess: Don't leak the AC flag into __put_user() value evaluation

When calling __put_user(foo(), ptr), the __put_user() macro would call
foo() in between __uaccess_begin() and __uaccess_end().  If that code
were buggy, then those bugs would be run without SMAP protection.

Fortunately, there seem to be few instances of the problem in the
kernel. Nevertheless, __put_user() should be fixed to avoid doing this.
Therefore, evaluate __put_user()'s argument before setting AC.

This issue was noticed when an objtool hack by Peter Zijlstra complained
about genregs_get() and I compared the assembly output to the C source.

 [ bp: Massage commit message and fixed up whitespace. ]

Fixes: 11f1a4b9755f ("x86: reorganize SMAP handling in user space accesses")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/20190225125231.845656645@infradead.org
---
 arch/x86/include/asm/uaccess.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a77445d1b034..28376aa2d053 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -284,7 +284,7 @@ do {									\
 		__put_user_goto(x, ptr, "l", "k", "ir", label);		\
 		break;							\
 	case 8:								\
-		__put_user_goto_u64((__typeof__(*ptr))(x), ptr, label);	\
+		__put_user_goto_u64(x, ptr, label);			\
 		break;							\
 	default:							\
 		__put_user_bad();					\
@@ -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();					\

Re: [PATCH 4/6] objtool: Replace STACK_FRAME_NON_STANDARD annotation

[Peter Zijlstra]

On Mon, Feb 25, 2019 at 10:11:24AM -0600, Josh Poimboeuf wrote:
> On Mon, Feb 25, 2019 at 01:43:34PM +0100, Peter Zijlstra wrote:

> > -#define STACK_FRAME_NON_STANDARD(func) \
> > -	static void __used __section(.discard.func_stack_frame_non_standard) \
> > -		*__func_stack_frame_non_standard_##func = func
> > +#define STACK_FRAME_NON_STANDARD(func)					\
> > +	asm (".pushsection .discard.nonstd_frame_strtab, \"S\", @3\n\t"	\
> > +	     "999: .ascii \"" #func "\"\n\t"				\
> > +	     "     .byte 0\n\t"						\
> > +	     ".popsection\n\t"						\
> > +	     ".pushsection .discard.nonstd_frame\n\t"			\
> > +	     ".long 999b - .\n\t"					\
> > +	     ".popsection\n\t")
> > +
> 
> I don't think this will work in the case where GCC does an IPA
> optimization and renames the function (e.g., renames func to
> func.isra.1234), right?  That might be a deal breaker...

Or; as has been found by 0day; the whole function gets inlined and
the symbol no longer exists at all.

That's curable with a noinline, but all things considered, I think we
should go back to the old horrible scheme. Andy?

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

On Mon, Feb 25, 2019 at 01:43:35PM +0100, Peter Zijlstra wrote:
> It is important that UACCESS regions are as small as possible;
> furthermore the UACCESS state is not scheduled, so doing anything that
> might directly call into the scheduler will cause random code to be
> ran with UACCESS enabled.
> 
> Teach objtool too track UACCESS state and warn about any CALL made
> while UACCESS is enabled. This very much includes the __fentry__()
> tracing calls and __preempt_schedule() calls.
> 
> Note that exceptions _do_ save/restore the UACCESS state, and therefore
> they can drive preemption. This also means that all exception handlers
> must have an otherwise dedundant UACCESS disable instruction;
> therefore ignore this warning for !STT_FUNC code (exception handlers
> are not normal functions).
> 
> It also provides a UACCESS_SAFE() annotation which allows explicit
> annotation. This is meant to be used for future things like:
> unsafe_copy_{to,from}_user().
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

So KASAN is wildly unhappy..

I can't actually find any definitions of those functions, so I can't
very well mark the safe, even if we wanted to.

---

>> arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x59: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x6a: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x7b: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x8f: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xa3: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xb4: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xc5: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xdc: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xf0: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x101: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x112: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x123: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x134: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x145: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x156: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x167: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x177: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x192: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x1bc: call to __asan_store8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x1f2: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x212: call to __asan_store8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x1d8: call to force_valid_ss.isra.0() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x3f: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x50: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x61: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x75: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x89: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x9a: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xab: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xbc: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xcd: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xde: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xef: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x100: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x111: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x122: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x133: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x143: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x157: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x173: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x18e: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x1a8: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x1c2: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x1ee: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x217: call to __asan_load8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x618: call to __asan_load8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x630: call to __asan_loadN_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x681: call to __asan_load8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x6a0: call to __asan_load4_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x6bd: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x7cb: call to __asan_load8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x6dd: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x6f0: call to __asan_store8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x707: call to __asan_store4_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x2c6: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x2e3: call to __asan_loadN_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x334: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x352: call to __asan_load4_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x374: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x49d: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x393: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x3aa: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x3c1: call to __asan_store4_noabort() with UACCESS enabled

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Andrey Ryabinin]

On 2/27/19 5:08 PM, Peter Zijlstra wrote:
> On Mon, Feb 25, 2019 at 01:43:35PM +0100, Peter Zijlstra wrote:
>> It is important that UACCESS regions are as small as possible;
>> furthermore the UACCESS state is not scheduled, so doing anything that
>> might directly call into the scheduler will cause random code to be
>> ran with UACCESS enabled.
>>
>> Teach objtool too track UACCESS state and warn about any CALL made
>> while UACCESS is enabled. This very much includes the __fentry__()
>> tracing calls and __preempt_schedule() calls.
>>
>> Note that exceptions _do_ save/restore the UACCESS state, and therefore
>> they can drive preemption. This also means that all exception handlers
>> must have an otherwise dedundant UACCESS disable instruction;
>> therefore ignore this warning for !STT_FUNC code (exception handlers
>> are not normal functions).
>>
>> It also provides a UACCESS_SAFE() annotation which allows explicit
>> annotation. This is meant to be used for future things like:
>> unsafe_copy_{to,from}_user().
>>
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> So KASAN is wildly unhappy..
> 
> I can't actually find any definitions of those functions, so I can't
> very well mark the safe, even if we wanted to.
> 

They are macro-generated. Use 'git grep DEFINE_ASAN'

> ---
> 
>>> arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x59: call to __asan_store8_noabort() with UACCESS enabled

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

On Wed, Feb 27, 2019 at 05:17:58PM +0300, Andrey Ryabinin wrote:
> 
> 
> On 2/27/19 5:08 PM, Peter Zijlstra wrote:

> > I can't actually find any definitions of those functions, so I can't
> > very well mark the safe, even if we wanted to.
> > 
> 
> They are macro-generated. Use 'git grep DEFINE_ASAN'

Ah, indeed! I'll go have a look.

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

On Wed, Feb 27, 2019 at 03:26:23PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 27, 2019 at 05:17:58PM +0300, Andrey Ryabinin wrote:
> > 
> > 
> > On 2/27/19 5:08 PM, Peter Zijlstra wrote:
> 
> > > I can't actually find any definitions of those functions, so I can't
> > > very well mark the safe, even if we wanted to.
> > > 
> > 
> > They are macro-generated. Use 'git grep DEFINE_ASAN'
> 
> Ah, indeed! I'll go have a look.

Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
'never' hit that, but it does leave us up a creek without a paddle.

Not sure what to do here; best I can come up with atm. is kill SMAP on
KASAN builds.

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Dmitry Vyukov]

On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Feb 27, 2019 at 03:26:23PM +0100, Peter Zijlstra wrote:
> > On Wed, Feb 27, 2019 at 05:17:58PM +0300, Andrey Ryabinin wrote:
> > >
> > >
> > > On 2/27/19 5:08 PM, Peter Zijlstra wrote:
> >
> > > > I can't actually find any definitions of those functions, so I can't
> > > > very well mark the safe, even if we wanted to.
> > > >
> > >
> > > They are macro-generated. Use 'git grep DEFINE_ASAN'
> >
> > Ah, indeed! I'll go have a look.
>
> Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> 'never' hit that, but it does leave us up a creek without a paddle.
>
> Not sure what to do here; best I can come up with atm. is kill SMAP on
> KASAN builds.

If SMAP detects additional bugs, then it would be pity to disable it
with KASAN (detect bugs in production but not during testing).

You mentioned that exception save/restore the UACCESS state. Is it
possible to do the same in kasan_report? At the very least we need to
survive report printing, what happens after that does not matter much
(we've corrupted memory by now anyway).

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Linus Torvalds]

On Wed, Feb 27, 2019 at 6:08 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> So KASAN is wildly unhappy..

Yeah, well, with KASAN you definitely end up doing lots and lots for
calls for just regular memory accesses.

Which we obviously need to do for most uaccess cases.

I think you should just say "ok, kasan reporting will possibly run with AC on".

Again, having AC on isn't fatal. It just makes the window where you
can incorrectly access user space through a wild pointer bigger.

So the whole "run with AC on" thing isn't a big deal, apart from the
scheduling case.

And we know how to fix the scheduling case by just doing the
save/restore thing..

                   Linus

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

On Wed, Feb 27, 2019 at 04:40:28PM +0100, Dmitry Vyukov wrote:
> On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <peterz@infradead.org> wrote:

> > Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> > 'never' hit that, but it does leave us up a creek without a paddle.

> If SMAP detects additional bugs, then it would be pity to disable it
> with KASAN (detect bugs in production but not during testing).
> 
> You mentioned that exception save/restore the UACCESS state. Is it
> possible to do the same in kasan_report? At the very least we need to
> survive report printing, what happens after that does not matter much
> (we've corrupted memory by now anyway).

Ideally we'll put all of kasan_report() in an exception, much like we do
for WARN. But there's a distinct lack of arch hooks there to play with.
I suppose I can try and create some.

On top of that we'll have to mark these __asan functions with notrace.

Maybe a little something horrible like so... completely untested.

---
 arch/x86/include/asm/bug.h   | 28 ++++++++++++++--------------
 arch/x86/include/asm/kasan.h | 11 +++++++++++
 include/asm-generic/bug.h    |  1 +
 include/linux/kasan.h        |  6 +++++-
 lib/bug.c                    |  8 +++++++-
 mm/kasan/generic.c           |  4 ++--
 mm/kasan/kasan.h             |  2 +-
 mm/kasan/report.c            |  2 +-
 8 files changed, 42 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 6804d6642767..ead3e6ad4eb6 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -30,33 +30,33 @@
 
 #ifdef CONFIG_DEBUG_BUGVERBOSE
 
-#define _BUG_FLAGS(ins, flags)						\
+#define _BUG_FLAGS(ins, flags, ...)					\
 do {									\
 	asm volatile("1:\t" ins "\n"					\
 		     ".pushsection __bug_table,\"aw\"\n"		\
-		     "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n"	\
-		     "\t"  __BUG_REL(%c0) "\t# bug_entry::file\n"	\
-		     "\t.word %c1"        "\t# bug_entry::line\n"	\
-		     "\t.word %c2"        "\t# bug_entry::flags\n"	\
-		     "\t.org 2b+%c3\n"					\
+		     "2:\t" __BUG_REL(1b)      "\t# bug_entry::bug_addr\n" \
+		     "\t"  __BUG_REL(%c[file]) "\t# bug_entry::file\n"	\
+		     "\t.word %c[line]"        "\t# bug_entry::line\n"	\
+		     "\t.word %c[flags]"       "\t# bug_entry::flags\n"	\
+		     "\t.org 2b+%c[size]\n"				\
 		     ".popsection"					\
-		     : : "i" (__FILE__), "i" (__LINE__),		\
-			 "i" (flags),					\
-			 "i" (sizeof(struct bug_entry)));		\
+		     : : [file] "i" (__FILE__), [line] "i" (__LINE__),	\
+			 [flags] "i" (flags),				\
+			 [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__);	\
 } while (0)
 
 #else /* !CONFIG_DEBUG_BUGVERBOSE */
 
-#define _BUG_FLAGS(ins, flags)						\
+#define _BUG_FLAGS(ins, flags, ...)					\
 do {									\
 	asm volatile("1:\t" ins "\n"					\
 		     ".pushsection __bug_table,\"aw\"\n"		\
 		     "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n"	\
-		     "\t.word %c0"        "\t# bug_entry::flags\n"	\
-		     "\t.org 2b+%c1\n"					\
+		     "\t.word %c[flags]"  "\t# bug_entry::flags\n"	\
+		     "\t.org 2b+%c[size]\n"				\
 		     ".popsection"					\
-		     : : "i" (flags),					\
-			 "i" (sizeof(struct bug_entry)));		\
+		     : : [flags] "i" (flags),				\
+			 [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__);	\
 } while (0)
 
 #endif /* CONFIG_DEBUG_BUGVERBOSE */
diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h
index 13e70da38bed..b7d563965b4d 100644
--- a/arch/x86/include/asm/kasan.h
+++ b/arch/x86/include/asm/kasan.h
@@ -2,6 +2,8 @@
 #ifndef _ASM_X86_KASAN_H
 #define _ASM_X86_KASAN_H
 
+#include <asm/bug.h>
+
 #include <linux/const.h>
 #define KASAN_SHADOW_OFFSET _AC(CONFIG_KASAN_SHADOW_OFFSET, UL)
 #define KASAN_SHADOW_SCALE_SHIFT 3
@@ -26,8 +28,17 @@
 #ifndef __ASSEMBLY__
 
 #ifdef CONFIG_KASAN
+
 void __init kasan_early_init(void);
 void __init kasan_init(void);
+
+static __always_inline void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
+{
+	unsigned long rdi = addr, rsi = size, rdx = is_write, rdx = ip;
+
+	_BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN, "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
+}
+
 #else
 static inline void kasan_early_init(void) { }
 static inline void kasan_init(void) { }
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 0e9bd9c83870..6c829e80ea2a 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -10,6 +10,7 @@
 #define BUGFLAG_WARNING		(1 << 0)
 #define BUGFLAG_ONCE		(1 << 1)
 #define BUGFLAG_DONE		(1 << 2)
+#define BUGFLAG_KASAN		(1 << 3)
 #define BUGFLAG_TAINT(taint)	((taint) << 8)
 #define BUG_GET_TAINT(bug)	((bug)->flags >> 8)
 #endif
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b40ea104dd36..0ab50faedc9e 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -177,7 +177,7 @@ void kasan_init_tags(void);
 
 void *kasan_reset_tag(const void *addr);
 
-void kasan_report(unsigned long addr, size_t size,
+void __kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip);
 
 #else /* CONFIG_KASAN_SW_TAGS */
@@ -191,4 +191,8 @@ static inline void *kasan_reset_tag(const void *addr)
 
 #endif /* CONFIG_KASAN_SW_TAGS */
 
+#ifndef kasan_report
+#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
+#endif
+
 #endif /* LINUX_KASAN_H */
diff --git a/lib/bug.c b/lib/bug.c
index 1077366f496b..c9c005a35b76 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -144,7 +144,7 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 {
 	struct bug_entry *bug;
 	const char *file;
-	unsigned line, warning, once, done;
+	unsigned line, warning, once, done, kasan;
 
 	if (!is_valid_bugaddr(bugaddr))
 		return BUG_TRAP_TYPE_NONE;
@@ -167,6 +167,7 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 		line = bug->line;
 #endif
 		warning = (bug->flags & BUGFLAG_WARNING) != 0;
+		kasan = (bug->flags & BUGFLAG_KASAN) != 0;
 		once = (bug->flags & BUGFLAG_ONCE) != 0;
 		done = (bug->flags & BUGFLAG_DONE) != 0;
 
@@ -188,6 +189,11 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 		return BUG_TRAP_TYPE_WARN;
 	}
 
+	if (kasan) {
+		__kasan_report(regs->di, regs->si, regs->dx, regs->cx);
+		return BUG_TRAP_TYPE_WARN;
+	}
+
 	printk(KERN_DEFAULT CUT_HERE);
 
 	if (file)
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index ccb6207276e3..84be578e2591 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -228,7 +228,7 @@ void __asan_unregister_globals(struct kasan_global *globals, size_t size)
 EXPORT_SYMBOL(__asan_unregister_globals);
 
 #define DEFINE_ASAN_LOAD_STORE(size)					\
-	void __asan_load##size(unsigned long addr)			\
+	notrace void __asan_load##size(unsigned long addr)		\
 	{								\
 		check_memory_region_inline(addr, size, false, _RET_IP_);\
 	}								\
@@ -236,7 +236,7 @@ EXPORT_SYMBOL(__asan_unregister_globals);
 	__alias(__asan_load##size)					\
 	void __asan_load##size##_noabort(unsigned long);		\
 	EXPORT_SYMBOL(__asan_load##size##_noabort);			\
-	void __asan_store##size(unsigned long addr)			\
+	notrace void __asan_store##size(unsigned long addr)		\
 	{								\
 		check_memory_region_inline(addr, size, true, _RET_IP_);	\
 	}								\
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index ea51b2d898ec..7bc954b4fc2d 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -130,7 +130,7 @@ void check_memory_region(unsigned long addr, size_t size, bool write,
 void *find_first_bad_addr(void *addr, size_t size);
 const char *get_bug_type(struct kasan_access_info *info);
 
-void kasan_report(unsigned long addr, size_t size,
+void __kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip);
 void kasan_report_invalid_free(void *object, unsigned long ip);
 
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index ca9418fe9232..1d5050659388 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *object, unsigned long ip)
 	end_report(&flags);
 }
 
-void kasan_report(unsigned long addr, size_t size,
+void __kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip)
 {
 	struct kasan_access_info info;

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

On Wed, Feb 27, 2019 at 08:18:21AM -0800, Linus Torvalds wrote:

> So the whole "run with AC on" thing isn't a big deal, apart from the
> scheduling case.
> 
> And we know how to fix the scheduling case by just doing the
> save/restore thing..

Right, but I was hoping to avoid having to do that; and I think we can
do that if we push all of kasan_report into an exception, like that
'patch' I just posted.

If I'm not mistaken the regular kasan house-keeping crud just prods at
the shadow memory with simple code and should be perfectly safe.

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Linus Torvalds]

On Wed, Feb 27, 2019 at 9:30 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Right, but I was hoping to avoid having to do that; and I think we can
> do that if we push all of kasan_report into an exception, like that
> 'patch' I just posted.
>
> If I'm not mistaken the regular kasan house-keeping crud just prods at
> the shadow memory with simple code and should be perfectly safe.

Fair enough. I do agree it would be nice to not have the save/restore
simply because "it cannot matter" and we have the static tool to prove
that.

             Linus

Re: [PATCH 4/6] objtool: Replace STACK_FRAME_NON_STANDARD annotation

[Andy Lutomirski]

On Wed, Feb 27, 2019 at 4:21 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Feb 25, 2019 at 10:11:24AM -0600, Josh Poimboeuf wrote:
> > On Mon, Feb 25, 2019 at 01:43:34PM +0100, Peter Zijlstra wrote:
>
> > > -#define STACK_FRAME_NON_STANDARD(func) \
> > > -   static void __used __section(.discard.func_stack_frame_non_standard) \
> > > -           *__func_stack_frame_non_standard_##func = func
> > > +#define STACK_FRAME_NON_STANDARD(func)                                     \
> > > +   asm (".pushsection .discard.nonstd_frame_strtab, \"S\", @3\n\t" \
> > > +        "999: .ascii \"" #func "\"\n\t"                            \
> > > +        "     .byte 0\n\t"                                         \
> > > +        ".popsection\n\t"                                          \
> > > +        ".pushsection .discard.nonstd_frame\n\t"                   \
> > > +        ".long 999b - .\n\t"                                       \
> > > +        ".popsection\n\t")
> > > +
> >
> > I don't think this will work in the case where GCC does an IPA
> > optimization and renames the function (e.g., renames func to
> > func.isra.1234), right?  That might be a deal breaker...
>
> Or; as has been found by 0day; the whole function gets inlined and
> the symbol no longer exists at all.
>
> That's curable with a noinline, but all things considered, I think we
> should go back to the old horrible scheme. Andy?

Ugh, I guess.  I'm wondering just how atrocious the generated code is.

Just as a thought experiment, here are some other options:

1. Make a tiny GCC plugin that parses a special attribute on function
declarations and emits a record that describes that attribute into the
object file for each referenced symbol that comes from that
declaration.  (I know nothing about GCC internals, so I don't know how
hard this would be.)

2. Fiddle with the function names.  Turn a function called foo() into
__uaccess_safe_foo() and also emit a weak alias from that to foo.
This is probably every bit as bad as taking the address.

3. Take advantage of the fact that only static functions are (for now)
subject to this IPA stuff.  So take the address of a static function
or just declare that calling a static function is uaccess safe.

--Andy

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

On Wed, Feb 27, 2019 at 06:28:16PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 27, 2019 at 04:40:28PM +0100, Dmitry Vyukov wrote:
> > On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> > > 'never' hit that, but it does leave us up a creek without a paddle.
> 
> > If SMAP detects additional bugs, then it would be pity to disable it
> > with KASAN (detect bugs in production but not during testing).
> > 
> > You mentioned that exception save/restore the UACCESS state. Is it
> > possible to do the same in kasan_report? At the very least we need to
> > survive report printing, what happens after that does not matter much
> > (we've corrupted memory by now anyway).
> 
> Ideally we'll put all of kasan_report() in an exception, much like we do
> for WARN. But there's a distinct lack of arch hooks there to play with.
> I suppose I can try and create some.
> 
> On top of that we'll have to mark these __asan functions with notrace.
> 
> Maybe a little something horrible like so... completely untested.

OK, I got that to compile; the next problem is:

../include/linux/kasan.h:90:1: error: built-in function ‘__asan_loadN_noabort’ must be directly called
UACCESS_SAFE(__asan_loadN_noabort);

Which doesn't make any sense; since we actually generated that symbol,
it clearly is not built-in. What gives?

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Dmitry Vyukov]

On Thu, Feb 28, 2019 at 10:40 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Feb 27, 2019 at 06:28:16PM +0100, Peter Zijlstra wrote:
> > On Wed, Feb 27, 2019 at 04:40:28PM +0100, Dmitry Vyukov wrote:
> > > On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > > Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> > > > 'never' hit that, but it does leave us up a creek without a paddle.
> >
> > > If SMAP detects additional bugs, then it would be pity to disable it
> > > with KASAN (detect bugs in production but not during testing).
> > >
> > > You mentioned that exception save/restore the UACCESS state. Is it
> > > possible to do the same in kasan_report? At the very least we need to
> > > survive report printing, what happens after that does not matter much
> > > (we've corrupted memory by now anyway).
> >
> > Ideally we'll put all of kasan_report() in an exception, much like we do
> > for WARN. But there's a distinct lack of arch hooks there to play with.
> > I suppose I can try and create some.
> >
> > On top of that we'll have to mark these __asan functions with notrace.
> >
> > Maybe a little something horrible like so... completely untested.
>
> OK, I got that to compile; the next problem is:
>
> ../include/linux/kasan.h:90:1: error: built-in function ‘__asan_loadN_noabort’ must be directly called
> UACCESS_SAFE(__asan_loadN_noabort);
>
> Which doesn't make any sense; since we actually generated that symbol,
> it clearly is not built-in. What gives?

I guess this warning originated for user-space where programmer does
not define them and does not generally know about them and signature
is not a public contract for user. And then for kernel it just stayed
the same because not doing this warning would require somebody to
proactively think about this potential difference and add an
additional code to skip this check and even then it wasn't obvious why
one will want to do this with these functions. So that's where we are
now.

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Dmitry Vyukov]

On Thu, Feb 28, 2019 at 10:59 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Thu, Feb 28, 2019 at 10:40 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Feb 27, 2019 at 06:28:16PM +0100, Peter Zijlstra wrote:
> > > On Wed, Feb 27, 2019 at 04:40:28PM +0100, Dmitry Vyukov wrote:
> > > > On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > > > Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> > > > > 'never' hit that, but it does leave us up a creek without a paddle.
> > >
> > > > If SMAP detects additional bugs, then it would be pity to disable it
> > > > with KASAN (detect bugs in production but not during testing).
> > > >
> > > > You mentioned that exception save/restore the UACCESS state. Is it
> > > > possible to do the same in kasan_report? At the very least we need to
> > > > survive report printing, what happens after that does not matter much
> > > > (we've corrupted memory by now anyway).
> > >
> > > Ideally we'll put all of kasan_report() in an exception, much like we do
> > > for WARN. But there's a distinct lack of arch hooks there to play with.
> > > I suppose I can try and create some.
> > >
> > > On top of that we'll have to mark these __asan functions with notrace.
> > >
> > > Maybe a little something horrible like so... completely untested.
> >
> > OK, I got that to compile; the next problem is:
> >
> > ../include/linux/kasan.h:90:1: error: built-in function ‘__asan_loadN_noabort’ must be directly called
> > UACCESS_SAFE(__asan_loadN_noabort);
> >
> > Which doesn't make any sense; since we actually generated that symbol,
> > it clearly is not built-in. What gives?
>
> I guess this warning originated for user-space where programmer does
> not define them and does not generally know about them and signature
> is not a public contract for user. And then for kernel it just stayed
> the same because not doing this warning would require somebody to
> proactively think about this potential difference and add an
> additional code to skip this check and even then it wasn't obvious why
> one will want to do this with these functions. So that's where we are
> now.

Maybe asm directive will help to trick the compiler?

Re: [PATCH 5/6] objtool: Add UACCESS validation

[Peter Zijlstra]

On Thu, Feb 28, 2019 at 11:05:10AM +0100, Dmitry Vyukov wrote:
> On Thu, Feb 28, 2019 at 10:59 AM Dmitry Vyukov <dvyukov@google.com> wrote:

> > I guess this warning originated for user-space where programmer does
> > not define them and does not generally know about them and signature
> > is not a public contract for user. And then for kernel it just stayed
> > the same because not doing this warning would require somebody to
> > proactively think about this potential difference and add an
> > additional code to skip this check and even then it wasn't obvious why
> > one will want to do this with these functions. So that's where we are
> > now.
> 
> Maybe asm directive will help to trick the compiler?

So I went back and forth on the annotation; and we're back to the same
we use for STACK_FRAME_NON_STANDARD() because that forces GCC to
actually generate the symbol.

Without that GCC IPA will go and wreck things by either making the
entire symbol go away or generating partial functions.

I'm currently doing a hard-coded list of names in objtool for this :/
But I'm having trouble with that alias crud.