[PATCH 0/8] objtool: UACCESS validation v2

[PATCH 0/8] objtool: UACCESS validation v2

[Peter Zijlstra]

Teach objtool to validate the UACCESS (SMAP, PAN) rules with 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()).

This set now compiles x86_64-allmodconfig _almost_ clean:

  arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x30: redundant UACCESS disable
  drivers/xen/privcmd.o: warning: objtool: privcmd_ioctl()+0x1c0: call to {dynamic}() with UACCESS enabled
  drivers/xen/privcmd.o: warning: objtool: privcmd_ioctl()+0x8f8: call to hypercall_page() with UACCESS enabled

Also; I found the UACCESS_SAFE() annotation as presented in these patches to be
inadequate; so I might go back to the STH_STRTAB variant for this.
Alternatively, we can simply keep the hard-coded list we have now. There really
should not be many more function on there.

*compile tested only*, esp. the KASAN changes have not been verified to
actually *work*.

---
 arch/x86/ia32/ia32_signal.c                |  29 +++--
 arch/x86/include/asm/bug.h                 |  28 ++--
 arch/x86/include/asm/kasan.h               |  15 +++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   3 +-
 include/asm-generic/bug.h                  |   1 +
 include/linux/frame.h                      |  23 ++++
 include/linux/kasan.h                      |  12 +-
 lib/bug.c                                  |   9 +-
 mm/kasan/generic.c                         |   4 +-
 mm/kasan/kasan.h                           |   2 +-
 mm/kasan/report.c                          |   2 +-
 tools/objtool/arch.h                       |   6 +-
 tools/objtool/arch/x86/decode.c            |  22 +++-
 tools/objtool/check.c                      | 197 +++++++++++++++++++++++------
 tools/objtool/check.h                      |   3 +-
 tools/objtool/elf.c                        |  15 ++-
 tools/objtool/elf.h                        |   3 +-
 17 files changed, 290 insertions(+), 84 deletions(-)

[PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
UACCESS context, and kasan_report() is most definitely _NOT_ safe to
be called from there, move it into an exception much like BUG/WARN.

*compile tested only*

Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/bug.h   |   28 ++++++++++++++--------------
 arch/x86/include/asm/kasan.h |   15 +++++++++++++++
 include/asm-generic/bug.h    |    1 +
 include/linux/kasan.h        |   12 +++++++++---
 lib/bug.c                    |    9 ++++++++-
 mm/kasan/generic.c           |    4 ++--
 mm/kasan/kasan.h             |    2 +-
 mm/kasan/report.c            |    2 +-
 8 files changed, 51 insertions(+), 22 deletions(-)

--- 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[flag]"        "\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__),	\
+			 [flag] "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[flag]"   "\t# bug_entry::flags\n"	\
+		     "\t.org 2b+%c[size]\n"				\
 		     ".popsection"					\
-		     : : "i" (flags),					\
-			 "i" (sizeof(struct bug_entry)));		\
+		     : : [flag] "i" (flags),				\
+			 [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__);	\
 } while (0)
 
 #endif /* CONFIG_DEBUG_BUGVERBOSE */
--- 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,21 @@
 #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, rcx = ip;
+
+	_BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN,
+		   "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
+	annotate_reachable();
+}
+#define kasan_report kasan_report
+
 #else
 static inline void kasan_early_init(void) { }
 static inline void kasan_init(void) { }
--- 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
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -83,6 +83,9 @@ size_t kasan_metadata_size(struct kmem_c
 bool kasan_save_enable_multi_shot(void);
 void kasan_restore_multi_shot(bool enabled);
 
+void __kasan_report(unsigned long addr, size_t size,
+		bool is_write, unsigned long ip);
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
@@ -153,8 +156,14 @@ static inline void kasan_remove_zero_sha
 static inline void kasan_unpoison_slab(const void *ptr) { }
 static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
 
+static inline void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) { }
+
 #endif /* CONFIG_KASAN */
 
+#ifndef kasan_report
+#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
+#endif
+
 #ifdef CONFIG_KASAN_GENERIC
 
 #define KASAN_SHADOW_INIT 0
@@ -177,9 +186,6 @@ void kasan_init_tags(void);
 
 void *kasan_reset_tag(const void *addr);
 
-void kasan_report(unsigned long addr, size_t size,
-		bool is_write, unsigned long ip);
-
 #else /* CONFIG_KASAN_SW_TAGS */
 
 static inline void kasan_init_tags(void) { }
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -47,6 +47,7 @@
 #include <linux/bug.h>
 #include <linux/sched.h>
 #include <linux/rculist.h>
+#include <linux/kasan.h>
 
 extern struct bug_entry __start___bug_table[], __stop___bug_table[];
 
@@ -144,7 +145,7 @@ enum bug_trap_type report_bug(unsigned l
 {
 	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 +168,7 @@ enum bug_trap_type report_bug(unsigned l
 		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 +190,11 @@ enum bug_trap_type report_bug(unsigned l
 		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)
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -228,7 +228,7 @@ void __asan_unregister_globals(struct ka
 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_);	\
 	}								\
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -130,7 +130,7 @@ void check_memory_region(unsigned long a
 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);
 
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *obj
 	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;

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

[Peter Zijlstra]

Objtool spotted that we call native_load_gs_index() with AC set.
Re-arrange the code to avoid that.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
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();

[PATCH 3/8] 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/8] objtool: Hande function aliases

[Peter Zijlstra]

Function aliases result in different symbols for the same set of
instructions; track a canonical symbol so we have unique point of
access.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/elf.c |   15 +++++++++++----
 tools/objtool/elf.h |    2 +-
 2 files changed, 12 insertions(+), 5 deletions(-)

--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -219,7 +219,7 @@ static int read_sections(struct elf *elf
 static int read_symbols(struct elf *elf)
 {
 	struct section *symtab, *sec;
-	struct symbol *sym, *pfunc;
+	struct symbol *sym, *pfunc, *alias;
 	struct list_head *entry, *tmp;
 	int symbols_nr, i;
 	char *coldstr;
@@ -239,6 +239,7 @@ static int read_symbols(struct elf *elf)
 			return -1;
 		}
 		memset(sym, 0, sizeof(*sym));
+		alias = sym;
 
 		sym->idx = i;
 
@@ -288,11 +289,17 @@ static int read_symbols(struct elf *elf)
 				break;
 			}
 
-			if (sym->offset == s->offset && sym->len >= s->len) {
-				entry = tmp;
-				break;
+			if (sym->offset == s->offset) {
+				if (sym->len == s->len && alias == sym)
+					alias = s;
+
+				if (sym->len >= s->len) {
+					entry = tmp;
+					break;
+				}
 			}
 		}
+		sym->alias = alias;
 		list_add(&sym->list, entry);
 		hash_add(sym->sec->symbol_hash, &sym->hash, sym->idx);
 	}
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -61,7 +61,7 @@ struct symbol {
 	unsigned char bind, type;
 	unsigned long offset;
 	unsigned int len;
-	struct symbol *pfunc, *cfunc;
+	struct symbol *pfunc, *cfunc, *alias;
 };
 
 struct rela {

[PATCH 5/8] objtool: Rewrite add_ignores()

[Peter Zijlstra]

The whole add_ignores() thing was wildly weird; rewrite it according
to 'modern' ways.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   51 +++++++++++++++++++-------------------------------
 tools/objtool/check.h |    1 
 2 files changed, 20 insertions(+), 32 deletions(-)

--- 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
@@ -436,18 +413,31 @@ static void add_ignores(struct objtool_f
 	struct instruction *insn;
 	struct section *sec;
 	struct symbol *func;
+	struct rela *rela;
 
-	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.func_stack_frame_non_standard");
+	if (!sec)
+		return;
 
-			if (!ignore_func(file, func))
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		switch (rela->sym->type) {
+		case STT_FUNC:
+			func = rela->sym;
+			break;
+
+		case STT_SECTION:
+			func = find_symbol_by_offset(rela->sym->sec, rela->addend);
+			if (!func || func->type != STT_FUNC)
 				continue;
+			break;
 
-			func_for_each_insn_all(file, func, insn)
-				insn->ignore = true;
+		default:
+			WARN("unexpected relation symbol type in %s: %d", sec->name, rela->sym->type);
+			continue;
 		}
+
+		func_for_each_insn_all(file, func, insn)
+			insn->ignore = true;
 	}
 }
 
@@ -2198,7 +2188,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 6/8] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x3c: redundant UACCESS disable
drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x66: redundant UACCESS disable

AKA. you don't need user_access_end() if user_access_begin() fails.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1605,7 +1605,6 @@ static int eb_copy_relocations(const str
 					     (char __user *)urelocs + copied,
 					     len)) {
 end_user:
-				user_access_end();
 				kvfree(relocs);
 				err = -EFAULT;
 				goto err;
@@ -2628,8 +2627,8 @@ i915_gem_execbuffer2_ioctl(struct drm_de
 					&user_exec_list[i].offset,
 					end_user);
 		}
-end_user:
 		user_access_end();
+end_user:;
 	}
 
 	args->flags &= ~__I915_EXEC_UNKNOWN_FLAGS;

[PATCH 7/8] 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           |   23 +++++++
 tools/objtool/arch.h            |    4 -
 tools/objtool/arch/x86/decode.c |   14 ++++
 tools/objtool/check.c           |  126 +++++++++++++++++++++++++++++++++++++---
 tools/objtool/check.h           |    2 
 tools/objtool/elf.h             |    1 
 6 files changed, 158 insertions(+), 12 deletions(-)

--- 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 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) \
+	static void __used __section(.discard.func_uaccess_safe) \
+		*__func_uaccess_safe_##func = func
+
+#else
+
+#define UACCESS_SAFE(func)
+
+#endif
+
 #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
@@ -441,6 +441,66 @@ static void add_ignores(struct objtool_f
 	}
 }
 
+static const char *uaccess_safe_builtin[] = {
+	/* KASAN */
+	"__asan_loadN_noabort",
+	"__asan_load1_noabort",
+	"__asan_load2_noabort",
+	"__asan_load4_noabort",
+	"__asan_load8_noabort",
+	"__asan_load16_noabort",
+	"__asan_storeN_noabort",
+	"__asan_store1_noabort",
+	"__asan_store2_noabort",
+	"__asan_store4_noabort",
+	"__asan_store8_noabort",
+	"__asan_store16_noabort",
+	/* KCOV */
+	"__sanitizer_cov_trace_pc",
+	/* misc */
+	"csum_partial_copy_generic",
+	"__memcpy_mcsafe",
+	NULL
+};
+
+static void add_uaccess_safe(struct objtool_file *file)
+{
+	struct section *sec;
+	struct symbol *func;
+	struct rela *rela;
+	const char **name;
+
+	sec = find_section_by_name(file->elf, ".rela.discard.func_uaccess_safe");
+	if (sec) {
+		list_for_each_entry(rela, &sec->rela_list, list) {
+			switch (rela->sym->type) {
+			case STT_FUNC:
+				func = rela->sym;
+				break;
+
+			case STT_SECTION:
+				func = find_symbol_by_offset(rela->sym->sec, rela->addend);
+				if (!func || func->type != STT_FUNC)
+					continue;
+				break;
+
+			default:
+				continue;
+			}
+
+			func->alias->uaccess_safe = true;
+		}
+	}
+
+	for (name = uaccess_safe_builtin; *name; name++) {
+		func = find_symbol_by_name(file->elf, *name);
+		if (!func)
+			continue;
+
+		func->alias->uaccess_safe = true;
+	}
+}
+
 /*
  * 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.
@@ -1229,6 +1289,7 @@ static int decode_sections(struct objtoo
 		return ret;
 
 	add_ignores(file);
+	add_uaccess_safe(file);
 
 	ret = add_nospec_ignores(file);
 	if (ret)
@@ -1789,6 +1850,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->alias->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
@@ -1893,6 +1970,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);
@@ -1908,17 +1988,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);
@@ -1971,6 +2058,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;
 		}
@@ -2132,6 +2238,8 @@ static int validate_functions(struct obj
 			if (!insn || insn->ignore)
 				continue;
 
+			state.uaccess_safe = func->alias->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, *alias;
+	bool uaccess_safe;
 };
 
 struct rela {

[PATCH 8/8] 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
@@ -1930,6 +1930,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);
@@ -1951,6 +1954,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))
@@ -2034,6 +2041,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 6/8] i915,uaccess: Fix redundant CLAC

[Chris Wilson]

Quoting Peter Zijlstra (2019-02-28 14:54:56)
> drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x3c: redundant UACCESS disable
> drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x66: redundant UACCESS disable
> 
> AKA. you don't need user_access_end() if user_access_begin() fails.

Cool, I had no idea if it was required or not. The closest to
instructions on how to use user_access_begin() is in
arch/x86/include/asm/uaccess.h ... which I guess is earlier in this
series!

> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> be called from there, move it into an exception much like BUG/WARN.
>
> *compile tested only*


Please test it by booting KASAN kernel and then loading module
produced by CONFIG_TEST_KASAN=y. There are too many subtle aspects to
rely on "compile tested only", reviewers can't catch all of them
either.


> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/bug.h   |   28 ++++++++++++++--------------
>  arch/x86/include/asm/kasan.h |   15 +++++++++++++++
>  include/asm-generic/bug.h    |    1 +
>  include/linux/kasan.h        |   12 +++++++++---
>  lib/bug.c                    |    9 ++++++++-
>  mm/kasan/generic.c           |    4 ++--
>  mm/kasan/kasan.h             |    2 +-
>  mm/kasan/report.c            |    2 +-
>  8 files changed, 51 insertions(+), 22 deletions(-)
>
> --- 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[flag]"        "\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__),  \
> +                        [flag] "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[flag]"   "\t# bug_entry::flags\n"      \
> +                    "\t.org 2b+%c[size]\n"                             \
>                      ".popsection"                                      \
> -                    : : "i" (flags),                                   \
> -                        "i" (sizeof(struct bug_entry)));               \
> +                    : : [flag] "i" (flags),                            \
> +                        [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__); \
>  } while (0)
>
>  #endif /* CONFIG_DEBUG_BUGVERBOSE */
> --- 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,21 @@
>  #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, rcx = ip;
> +
> +       _BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN,
> +                  "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));

Can BUG return? This should be able to return.
We also have other tools coming (KMSAN/KTSAN) where distinction
between fast path that does nothing and slower-paths are very blurred
and there are dozens of them, I don't think this BUG thunk will be
sustainable. What does BUG do what a normal call can't do?


> +       annotate_reachable();
> +}
> +#define kasan_report kasan_report
> +
>  #else
>  static inline void kasan_early_init(void) { }
>  static inline void kasan_init(void) { }
> --- 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
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -83,6 +83,9 @@ size_t kasan_metadata_size(struct kmem_c
>  bool kasan_save_enable_multi_shot(void);
>  void kasan_restore_multi_shot(bool enabled);
>
> +void __kasan_report(unsigned long addr, size_t size,
> +               bool is_write, unsigned long ip);
> +
>  #else /* CONFIG_KASAN */
>
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -153,8 +156,14 @@ static inline void kasan_remove_zero_sha
>  static inline void kasan_unpoison_slab(const void *ptr) { }
>  static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
>
> +static inline void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) { }
> +
>  #endif /* CONFIG_KASAN */
>
> +#ifndef kasan_report
> +#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
> +#endif
> +
>  #ifdef CONFIG_KASAN_GENERIC
>
>  #define KASAN_SHADOW_INIT 0
> @@ -177,9 +186,6 @@ void kasan_init_tags(void);
>
>  void *kasan_reset_tag(const void *addr);
>
> -void kasan_report(unsigned long addr, size_t size,
> -               bool is_write, unsigned long ip);
> -
>  #else /* CONFIG_KASAN_SW_TAGS */
>
>  static inline void kasan_init_tags(void) { }
> --- a/lib/bug.c
> +++ b/lib/bug.c
> @@ -47,6 +47,7 @@
>  #include <linux/bug.h>
>  #include <linux/sched.h>
>  #include <linux/rculist.h>
> +#include <linux/kasan.h>
>
>  extern struct bug_entry __start___bug_table[], __stop___bug_table[];
>
> @@ -144,7 +145,7 @@ enum bug_trap_type report_bug(unsigned l
>  {
>         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 +168,7 @@ enum bug_trap_type report_bug(unsigned l
>                 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 +190,11 @@ enum bug_trap_type report_bug(unsigned l
>                 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)
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -228,7 +228,7 @@ void __asan_unregister_globals(struct ka
>  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)              \


We already have:
CFLAGS_REMOVE_generic.o = -pg
Doesn't it imply notrace for all functions?



>         {                                                               \
>                 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_); \
>         }                                                               \
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -130,7 +130,7 @@ void check_memory_region(unsigned long a
>  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);
>
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *obj
>         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 6/8] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

On Thu, Feb 28, 2019 at 03:10:44PM +0000, Chris Wilson wrote:
> Quoting Peter Zijlstra (2019-02-28 14:54:56)
> > drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x3c: redundant UACCESS disable
> > drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x66: redundant UACCESS disable
> > 
> > AKA. you don't need user_access_end() if user_access_begin() fails.
> 
> Cool, I had no idea if it was required or not. The closest to
> instructions on how to use user_access_begin() is in
> arch/x86/include/asm/uaccess.h ... which I guess is earlier in this
> series!

Ah, I don't think I put the rules in a comment; that would've been
useful I suppose ;-)

I did teach the rules to objtool though, and that is in the next patch:

  https://lkml.kernel.org/r/20190228150152.635362492@infradead.org

so at least it will yell at you during compile time when you get it
wrong.

> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks!

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Thu, Feb 28, 2019 at 04:22:04PM +0100, Dmitry Vyukov wrote:
> On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> > UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> > be called from there, move it into an exception much like BUG/WARN.
> >
> > *compile tested only*
> 
> 
> Please test it by booting KASAN kernel and then loading module
> produced by CONFIG_TEST_KASAN=y. There are too many subtle aspects to
> rely on "compile tested only", reviewers can't catch all of them
> either.

Sure, I'll do that. I just wanted to share the rest of the patches.

A quick test shows it dies _REAAAAAAAALY_ early, as in:

"Booting the kernel."

is the first and very last thing it says... I wonder how I did that :-)

> > +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, rcx = ip;
> > +
> > +       _BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN,
> > +                  "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
> 
> Can BUG return?

Yes. Also see the annotate_reachable().

> This should be able to return.
> We also have other tools coming (KMSAN/KTSAN) where distinction
> between fast path that does nothing and slower-paths are very blurred
> and there are dozens of them, I don't think this BUG thunk will be
> sustainable. What does BUG do what a normal call can't do?

It keeps the SMAP validation rules nice and tight. If we were to add
(and allow) things like pushf;clac;call ponies;popf or similar things,
it all becomes complicated real quick.

How would KMSAN/KTSAN interact with SMAP ?

> > +       annotate_reachable();
> > +}
> > @@ -228,7 +228,7 @@ void __asan_unregister_globals(struct ka
> >  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)              \
> 
> 
> We already have:
> CFLAGS_REMOVE_generic.o = -pg
> Doesn't it imply notrace for all functions?

Indeed so, I'll make these hunks go away.

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Thu, Feb 28, 2019 at 4:46 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Feb 28, 2019 at 04:22:04PM +0100, Dmitry Vyukov wrote:
> > On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> > > UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> > > be called from there, move it into an exception much like BUG/WARN.
> > >
> > > *compile tested only*
> >
> >
> > Please test it by booting KASAN kernel and then loading module
> > produced by CONFIG_TEST_KASAN=y. There are too many subtle aspects to
> > rely on "compile tested only", reviewers can't catch all of them
> > either.
>
> Sure, I'll do that. I just wanted to share the rest of the patches.
>
> A quick test shows it dies _REAAAAAAAALY_ early, as in:
>
> "Booting the kernel."
>
> is the first and very last thing it says... I wonder how I did that :-)

One thing is that during early boot kasan_report is called multiple
times, but these are false positives related to the fact that we don't
have a proper shadow yet (setup later). So during early boot we set
kasan_disable=1 (or some global or per-task flag), and then
kasan_report checks it and returns.
Once we setup proper shadow, the flag is reset and from now on
kasan_report actually reports bug.


> > > +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, rcx = ip;
> > > +
> > > +       _BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN,
> > > +                  "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
> >
> > Can BUG return?
>
> Yes. Also see the annotate_reachable().
>
> > This should be able to return.
> > We also have other tools coming (KMSAN/KTSAN) where distinction
> > between fast path that does nothing and slower-paths are very blurred
> > and there are dozens of them, I don't think this BUG thunk will be
> > sustainable. What does BUG do what a normal call can't do?
>
> It keeps the SMAP validation rules nice and tight. If we were to add
> (and allow) things like pushf;clac;call ponies;popf or similar things,
> it all becomes complicated real quick.
>
> How would KMSAN/KTSAN interact with SMAP ?
>
> > > +       annotate_reachable();
> > > +}
> > > @@ -228,7 +228,7 @@ void __asan_unregister_globals(struct ka
> > >  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)              \
> >
> >
> > We already have:
> > CFLAGS_REMOVE_generic.o = -pg
> > Doesn't it imply notrace for all functions?
>
> Indeed so, I'll make these hunks go away.

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Andrey Ryabinin]

On 2/28/19 6:52 PM, Dmitry Vyukov wrote:
> On Thu, Feb 28, 2019 at 4:46 PM Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> On Thu, Feb 28, 2019 at 04:22:04PM +0100, Dmitry Vyukov wrote:
>>> On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
>>>>
>>>> Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
>>>> UACCESS context, and kasan_report() is most definitely _NOT_ safe to
>>>> be called from there, move it into an exception much like BUG/WARN.
>>>>
>>>> *compile tested only*
>>>
>>>
>>> Please test it by booting KASAN kernel and then loading module
>>> produced by CONFIG_TEST_KASAN=y. There are too many subtle aspects to
>>> rely on "compile tested only", reviewers can't catch all of them
>>> either.
>>
>> Sure, I'll do that. I just wanted to share the rest of the patches.
>>
>> A quick test shows it dies _REAAAAAAAALY_ early, as in:
>>
>> "Booting the kernel."
>>
>> is the first and very last thing it says... I wonder how I did that :-)
> 
> One thing is that during early boot kasan_report is called multiple
> times, but these are false positives related to the fact that we don't
> have a proper shadow yet (setup later). So during early boot we set
> kasan_disable=1 (or some global or per-task flag), and then
> kasan_report checks it and returns.
> Once we setup proper shadow, the flag is reset and from now on
> kasan_report actually reports bug.
> 

Yup, see report_enabled() function.

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Thu, Feb 28, 2019 at 4:46 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Feb 28, 2019 at 04:22:04PM +0100, Dmitry Vyukov wrote:
> > On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> > > UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> > > be called from there, move it into an exception much like BUG/WARN.
> > >
> > > *compile tested only*
> >
> >
> > Please test it by booting KASAN kernel and then loading module
> > produced by CONFIG_TEST_KASAN=y. There are too many subtle aspects to
> > rely on "compile tested only", reviewers can't catch all of them
> > either.
>
> Sure, I'll do that. I just wanted to share the rest of the patches.
>
> A quick test shows it dies _REAAAAAAAALY_ early, as in:
>
> "Booting the kernel."
>
> is the first and very last thing it says... I wonder how I did that :-)
>
> > > +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, rcx = ip;
> > > +
> > > +       _BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN,
> > > +                  "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
> >
> > Can BUG return?
>
> Yes. Also see the annotate_reachable().
>
> > This should be able to return.
> > We also have other tools coming (KMSAN/KTSAN) where distinction
> > between fast path that does nothing and slower-paths are very blurred
> > and there are dozens of them, I don't think this BUG thunk will be
> > sustainable. What does BUG do what a normal call can't do?
>
> It keeps the SMAP validation rules nice and tight. If we were to add
> (and allow) things like pushf;clac;call ponies;popf or similar things,
> it all becomes complicated real quick.
>
> How would KMSAN/KTSAN interact with SMAP ?


I am missing some knowledge about SMAP to answer this.
In short, these tools insert lots of callbacks into runtime for memory
accesses, function entry/exit, atomicops and some other. These
callbacks can do things of different complexity.
Humm... perhaps we could just disable SMAP for KMSAN/KTSAN. It's
possible, right? If we have it enabled with KASAN, that should be
enough.

And the question from another thread: does SMAP detect bugs that KASAN
can't? If SMAP is not useful during stress testing/fuzzing, then we
could also disable it. But if it finds some bugs that KASAN won't
detect, then it would be pity to disable it.

Also, what's the actual problem with KASAN+SMAP? Is it warnings from
static analysis tool? Or there are also some runtime effects? What
effects?
Is it possible to disable the SMAP runtime checks once we enter
kasan_report() past report_enabled() check? We could restrict it to
"just finish printing this bug report whatever it takes and then
whatever" if it makes things simpler.
It would be nice if we could restrict it to something like:

@@ -291,6 +303,7 @@ void kasan_report(unsigned long addr, size_t size,
        if (likely(!report_enabled()))
                return;
+       disable_smap();

And then enforce panic at the end of report if smap is enabled.


> > > +       annotate_reachable();
> > > +}
> > > @@ -228,7 +228,7 @@ void __asan_unregister_globals(struct ka
> > >  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)              \
> >
> >
> > We already have:
> > CFLAGS_REMOVE_generic.o = -pg
> > Doesn't it imply notrace for all functions?
>
> Indeed so, I'll make these hunks go away.

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Linus Torvalds]

On Thu, Feb 28, 2019 at 7:05 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x3c: redundant UACCESS disable
> drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x66: redundant UACCESS disable
>
> AKA. you don't need user_access_end() if user_access_begin() fails.

NOOO!

This is complete garbage, and will end up running with AC set forever after.

PeterZ, you need to remove that "redundant UACCESS disable" check, or
make it a whole lot smarter. Because as it is, it's horribly horribly
wrong.

We absolutely _have_ to have that "user_access_end()" there.

Yes, it is redundant (but harmlessly so) if no fault occurs.

But if a fault occurs, that "user_access_end()" is what clears AC on
the faulting path. That's absolutely required, because we don't clear
it on return from exception (and we shouldn't - one common pattern for
user space exceptions is "try to do a big access, if that fails go
back to using small accesses until it fails again").

Your reachability clearly doesn't take exception handling into account.

That patch must die, and your incorrect reachability model must be fixed.

Right now the objtool rules seem to be worse than not using objtool,
if it causes these kinds of false positives.

              Linus

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Thu, Feb 28, 2019 at 05:03:09PM +0100, Dmitry Vyukov wrote:

> I am missing some knowledge about SMAP to answer this.
> In short, these tools insert lots of callbacks into runtime for memory
> accesses, function entry/exit, atomicops and some other. These
> callbacks can do things of different complexity.
> Humm... perhaps we could just disable SMAP for KMSAN/KTSAN. It's
> possible, right? If we have it enabled with KASAN, that should be
> enough.

SMAP detects access to _PAGE_USER pages; that is, such access is only
allowed when EFLAGS.AC=1, otherwise they'll fault.

I again don't know enough about KASAN to say if it does that; but I
suspect it only tracks kernel memory state.

> Also, what's the actual problem with KASAN+SMAP? Is it warnings from
> static analysis tool? Or there are also some runtime effects? What
> effects?

Both; so because of the above semantics, things like copy_to_user() will
have to do STAC (set EFLAGS.AC=1), then do the actual copies to the user
addresses, and then CLAC (clear the AC flag again).

The desire is to have AC=1 sections as small as possible, such that as
much code as possible is ran with AC=0 and will trap on unintended
accesses.

Also; the scheduler doesn't (but I have a patch for that, but I'd prefer
to not have to use it) context switch EFLAGS. This means that if we land
in the scheduler while AC=1, the next task will resume with AC=1.

Consequently, if that task returns to userspace before it gets scheduled
again, we'll continue our previous task (that left with AC=1) with AC=0
and it'll then fault where no fault were expected.

Anyway; the objtool annotation basically tracks the EFLAGS.AC state
(through STAC/CLAC instructions -- no PUSHF/POPF) and disallows any
CALL/RET while AC=1.

This is where the __asan_{load,store}*() stuff went *splat*. GCC inserts
those calls in the middle of STAC/CLAC (AC=1) and we then have to mark
the functions as AC-safe. objtool validates those on the same rules, no
further CALLs that are not also safe.

Things like __fentry__ are inherently unsafe because they use
preempt_disable/preempt_enable, where the latter has a CALL
__preempt_schedule (and is thus very unsafe). Similarly with
kasan_report(), it does all sorts of things that are not safe to do.

> Is it possible to disable the SMAP runtime checks once we enter
> kasan_report() past report_enabled() check? We could restrict it to
> "just finish printing this bug report whatever it takes and then
> whatever" if it makes things simpler.
> It would be nice if we could restrict it to something like:
> 
> @@ -291,6 +303,7 @@ void kasan_report(unsigned long addr, size_t size,
>         if (likely(!report_enabled()))
>                 return;
> +       disable_smap();
> 
> And then enforce panic at the end of report if smap is enabled.

That would be a CLAC, and the current rules disallow CLAC for AC-safe
functions.

Furthermore, kasan_report() isn't fatal, right? So it would have to
restore the state on exit. That makes the validation state much more
complicated.

Let me try and frob some of the report_enabled() stuff before the #UD.

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

On Thu, Feb 28, 2019 at 08:49:57AM -0800, Linus Torvalds wrote:
> Yes, it is redundant (but harmlessly so) if no fault occurs.

Oooh, there's a label hidden in unsafe_put_user()...

And yes, objtool seems to miss that, damn. I'll go stare at that.

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

On Thu, Feb 28, 2019 at 06:51:14PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 28, 2019 at 08:49:57AM -0800, Linus Torvalds wrote:
> > Yes, it is redundant (but harmlessly so) if no fault occurs.
> 
> Oooh, there's a label hidden in unsafe_put_user()...
> 
> And yes, objtool seems to miss that, damn. I'll go stare at that.

Weird, that jump is from C, not from a .fixup table. objtool _should_
see that and complain if there is a AC=1 path that reaches RET.

AFAIU validate_branch() should recurse down every single branch, right
Josh?

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Thu, Feb 28, 2019 at 6:46 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Feb 28, 2019 at 05:03:09PM +0100, Dmitry Vyukov wrote:
>
> > I am missing some knowledge about SMAP to answer this.
> > In short, these tools insert lots of callbacks into runtime for memory
> > accesses, function entry/exit, atomicops and some other. These
> > callbacks can do things of different complexity.
> > Humm... perhaps we could just disable SMAP for KMSAN/KTSAN. It's
> > possible, right? If we have it enabled with KASAN, that should be
> > enough.
>
> SMAP detects access to _PAGE_USER pages; that is, such access is only
> allowed when EFLAGS.AC=1, otherwise they'll fault.
>
> I again don't know enough about KASAN to say if it does that; but I
> suspect it only tracks kernel memory state.
>
> > Also, what's the actual problem with KASAN+SMAP? Is it warnings from
> > static analysis tool? Or there are also some runtime effects? What
> > effects?
>
> Both; so because of the above semantics, things like copy_to_user() will
> have to do STAC (set EFLAGS.AC=1), then do the actual copies to the user
> addresses, and then CLAC (clear the AC flag again).
>
> The desire is to have AC=1 sections as small as possible, such that as
> much code as possible is ran with AC=0 and will trap on unintended
> accesses.
>
> Also; the scheduler doesn't (but I have a patch for that, but I'd prefer
> to not have to use it) context switch EFLAGS. This means that if we land
> in the scheduler while AC=1, the next task will resume with AC=1.
>
> Consequently, if that task returns to userspace before it gets scheduled
> again, we'll continue our previous task (that left with AC=1) with AC=0
> and it'll then fault where no fault were expected.
>
> Anyway; the objtool annotation basically tracks the EFLAGS.AC state
> (through STAC/CLAC instructions -- no PUSHF/POPF) and disallows any
> CALL/RET while AC=1.
>
> This is where the __asan_{load,store}*() stuff went *splat*. GCC inserts
> those calls in the middle of STAC/CLAC (AC=1) and we then have to mark
> the functions as AC-safe. objtool validates those on the same rules, no
> further CALLs that are not also safe.
>
> Things like __fentry__ are inherently unsafe because they use
> preempt_disable/preempt_enable, where the latter has a CALL
> __preempt_schedule (and is thus very unsafe). Similarly with
> kasan_report(), it does all sorts of things that are not safe to do.
>
> > Is it possible to disable the SMAP runtime checks once we enter
> > kasan_report() past report_enabled() check? We could restrict it to
> > "just finish printing this bug report whatever it takes and then
> > whatever" if it makes things simpler.
> > It would be nice if we could restrict it to something like:
> >
> > @@ -291,6 +303,7 @@ void kasan_report(unsigned long addr, size_t size,
> >         if (likely(!report_enabled()))
> >                 return;
> > +       disable_smap();
> >
> > And then enforce panic at the end of report if smap is enabled.
>
> That would be a CLAC, and the current rules disallow CLAC for AC-safe
> functions.
>
> Furthermore, kasan_report() isn't fatal, right? So it would have to
> restore the state on exit. That makes the validation state much more
> complicated.
>
> Let me try and frob some of the report_enabled() stuff before the #UD.

What if do CLAC in the beginning of kasan_report, STAC at the end if
AC was set on entry. And then special-case kasan_report in the static
tool? This looks like a reasonable compromise to me taking into
account that KASAN is not enabled in production builds, so special
casing it in the static tool won't lead to missed bugs in production
code. That's effectively what _BUG_FLAGS will do, right? But just
without involving asm.
Or, perhaps, wrap it into something like:
void smap_call(void (*fn)(void* ctx), void *ctx);
that will do all of this. And then the tool only needs to know about
the smap_call?

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Linus Torvalds]

On Thu, Feb 28, 2019 at 10:02 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Weird, that jump is from C, not from a .fixup table. objtool _should_
> see that and complain if there is a AC=1 path that reaches RET.

No, unsafe_put_user() actually does the "asm goto" thing, so the jump
is literally hidden as an exception entry. And apparently objtool
doesn't follow exceptions (which *normally* doesn't matter for code
liveness analysis since they normally jump back to right after the
excepting instruction, but maybe it misses some exception handling
code because of it?).

You may have looked at unsafe_get_user(), which does indeed make the
branch as C code, because gcc currently does not allow outputs from
"asm goto" statements (which "get" obviously needs).

[ One of these days I really should look at the gcc sources to try to
figure out why gcc doesn't like them. I wish we could have a rule like
"it's an output only for the fallthrough case, not for the goto
cases". Because I wonder if the gcc peoples aversion to "asm goto" and
outputs comes from "we can't set outputs in multiple places". But my
gcc-foo is not strong enough that I've felt confident enough to really
go take a deep dive into something that feels pretty subtle, so I've
_thought_ about doing it for a long time, but have never actually
built up the confidence to do so ]

                    Linus

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

On Thu, Feb 28, 2019 at 10:29:25AM -0800, Linus Torvalds wrote:
> On Thu, Feb 28, 2019 at 10:02 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Weird, that jump is from C, not from a .fixup table. objtool _should_
> > see that and complain if there is a AC=1 path that reaches RET.
> 
> No, unsafe_put_user() actually does the "asm goto" thing, so the jump
> is literally hidden as an exception entry. And apparently objtool
> doesn't follow exceptions (which *normally* doesn't matter for code
> liveness analysis since they normally jump back to right after the
> excepting instruction, but maybe it misses some exception handling
> code because of it?).
> 
> You may have looked at unsafe_get_user(), which does indeed make the
> branch as C code, because gcc currently does not allow outputs from
> "asm goto" statements (which "get" obviously needs).

Indeed I did. But it looks like objtool actually does parse .fixup. What
appears to go wrong is the 'visited' marker for backward jumps.

If we've been there with AC=0 first, and then backjump with AC=1, things
go missing.

I've also now confused myself on how it branches from alternatives. It
looks like it now considers paths that take the STAC alternative, and
exit through the NOP alternative (which should be CLAC) and then hit
RET with AC=1.

I'll get this sorted, eventually..

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

On Thu, Feb 28, 2019 at 08:01:11PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 28, 2019 at 10:29:25AM -0800, Linus Torvalds wrote:
> > On Thu, Feb 28, 2019 at 10:02 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > Weird, that jump is from C, not from a .fixup table. objtool _should_
> > > see that and complain if there is a AC=1 path that reaches RET.
> > 
> > No, unsafe_put_user() actually does the "asm goto" thing, so the jump
> > is literally hidden as an exception entry. And apparently objtool
> > doesn't follow exceptions (which *normally* doesn't matter for code
> > liveness analysis since they normally jump back to right after the
> > excepting instruction, but maybe it misses some exception handling
> > code because of it?).
> > 
> > You may have looked at unsafe_get_user(), which does indeed make the
> > branch as C code, because gcc currently does not allow outputs from
> > "asm goto" statements (which "get" obviously needs).
> 
> Indeed I did. But it looks like objtool actually does parse .fixup. What
> appears to go wrong is the 'visited' marker for backward jumps.
> 
> If we've been there with AC=0 first, and then backjump with AC=1, things
> go missing.
> 
> I've also now confused myself on how it branches from alternatives. It
> looks like it now considers paths that take the STAC alternative, and
> exit through the NOP alternative (which should be CLAC) and then hit
> RET with AC=1.
> 
> I'll get this sorted, eventually..

Ha!

Original file:

CC      drivers/gpu/drm/i915/i915_gem_execbuffer.o
drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x3c: redundant UACCESS disable
drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x66: redundant UACCESS disable

With the dodgy patch:

CC      drivers/gpu/drm/i915/i915_gem_execbuffer.o
drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: eb_relocate_slow()+0x1f9: call to kvfree() with UACCESS enabled
drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: i915_gem_execbuffer2_ioctl()+0x315: call to kvfree() with UACCESS enabled


Let me do an allmodconfig build to see how much pain is caused by that
redundant CLAC warning.

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

On Fri, Mar 01, 2019 at 11:34:52AM +0100, Peter Zijlstra wrote:

> Let me do an allmodconfig build to see how much pain is caused by that
> redundant CLAC warning.

arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x30: redundant UACCESS disable
drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x22: redundant UACCESS disable
drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x64: redundant UACCESS disable
drivers/xen/privcmd.o: warning: objtool: privcmd_ioctl()+0x1c0: call to {dynamic}() with UACCESS enabled

The usercopy one is difficult, that's copy_user_handle_tail(), it is
buggered though, because that lacks notrace and thus has a __fentry__
call in.

Also, afaict all exception jumps into copy_user_handle_tail() will have
AC=1, but the __{get,put}_user_nocheck() things do STAC/CLAC all over
again.

So what do we do? Annotate that we start with AC=1 and then immediately
do the clac, and then let __{get,put}_user_nocheck() do their own thing?
or make it use the unsafe stuff?

---
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index ee42bb0cbeb3..e1ab9a50937c 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -59,7 +59,7 @@ EXPORT_SYMBOL(clear_user);
  * Since protection fault in copy_from/to_user is not a normal situation,
  * it is not necessary to optimize tail handling.
  */
-__visible unsigned long
+__visible notrace unsigned long
 copy_user_handle_tail(char *to, char *from, unsigned len)
 {
 	for (; len; --len, to++) {
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 485b259127c3..695212c5bd07 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1606,6 +1606,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
 					     len)) {
 end_user:
 				user_access_end();
+end:
 				kvfree(relocs);
 				err = -EFAULT;
 				goto err;
@@ -1625,7 +1626,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
 		 * relocations were valid.
 		 */
 		if (!user_access_begin(urelocs, size))
-			goto end_user;
+			goto end;
 
 		for (copied = 0; copied < nreloc; copied++)
 			unsafe_put_user(-1,
@@ -2616,7 +2617,7 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
 		 * when we did the "copy_from_user()" above.
 		 */
 		if (!user_access_begin(user_exec_list, count * sizeof(*user_exec_list)))
-			goto end_user;
+			goto end;
 
 		for (i = 0; i < args->buffer_count; i++) {
 			if (!(exec2_list[i].offset & UPDATE))
@@ -2630,6 +2631,7 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
 		}
 end_user:
 		user_access_end();
+end:;
 	}
 
 	args->flags &= ~__I915_EXEC_UNKNOWN_FLAGS;

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

On Fri, Mar 01, 2019 at 01:27:45PM +0100, Peter Zijlstra wrote:
> arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x30: redundant UACCESS disable

> The usercopy one is difficult, that's copy_user_handle_tail(), it is
> buggered though, because that lacks notrace and thus has a __fentry__
> call in.
> 
> Also, afaict all exception jumps into copy_user_handle_tail() will have
> AC=1, but the __{get,put}_user_nocheck() things do STAC/CLAC all over
> again.
> 
> So what do we do? Annotate that we start with AC=1 and then immediately
> do the clac, and then let __{get,put}_user_nocheck() do their own thing?
> or make it use the unsafe stuff?

Or.. we move the thing to assembly. Of course, I suck at (writing) asm,
so the below is probably broken in various ways.

--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -208,9 +208,6 @@ __copy_from_user_flushcache(void *dst, c
 }
 
 unsigned long
-copy_user_handle_tail(char *to, char *from, unsigned len);
-
-unsigned long
 mcsafe_handle_tail(char *to, char *from, unsigned len);
 
 #endif /* _ASM_X86_UACCESS_64_H */
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -194,6 +194,29 @@ ENDPROC(copy_user_enhanced_fast_string)
 EXPORT_SYMBOL(copy_user_enhanced_fast_string)
 
 /*
+ * Try to copy last bytes and clear the rest if needed.
+ * Since protection fault in copy_from/to_user is not a normal situation,
+ * it is not necessary to optimize tail handling.
+ *
+ * Input:
+ * rdi destination
+ * rsi source
+ * rdx count
+ *
+ * Output:
+ * eax uncopied bytes or 0 if successful.
+ */
+ENTRY(copy_user_handle_tail)
+	movl %edx,%ecx
+1:	rep movsb
+2:	mov %ecx,%eax
+	ASM_CLAC
+	ret
+
+	_ASM_EXTABLE_UA(1b, 2b)
+END(copy_user_handle_tail)
+
+/*
  * copy_user_nocache - Uncached memory copy with exception handling
  * This will force destination out of cache for more performance.
  *
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -55,26 +55,6 @@ unsigned long clear_user(void __user *to
 EXPORT_SYMBOL(clear_user);
 
 /*
- * Try to copy last bytes and clear the rest if needed.
- * Since protection fault in copy_from/to_user is not a normal situation,
- * it is not necessary to optimize tail handling.
- */
-__visible unsigned long
-copy_user_handle_tail(char *to, char *from, unsigned len)
-{
-	for (; len; --len, to++) {
-		char c;
-
-		if (__get_user_nocheck(c, from++, sizeof(char)))
-			break;
-		if (__put_user_nocheck(c, to, sizeof(char)))
-			break;
-	}
-	clac();
-	return len;
-}
-
-/*
  * Similar to copy_user_handle_tail, probe for the write fault point,
  * but reuse __memcpy_mcsafe in case a new read error is encountered.
  * clac() is handled in _copy_to_iter_mcsafe().

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

On Fri, Mar 01, 2019 at 01:57:18PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 01, 2019 at 01:27:45PM +0100, Peter Zijlstra wrote:
> > arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x30: redundant UACCESS disable
> 
> > The usercopy one is difficult, that's copy_user_handle_tail(), it is
> > buggered though, because that lacks notrace and thus has a __fentry__
> > call in.
> > 
> > Also, afaict all exception jumps into copy_user_handle_tail() will have
> > AC=1, but the __{get,put}_user_nocheck() things do STAC/CLAC all over
> > again.
> > 
> > So what do we do? Annotate that we start with AC=1 and then immediately
> > do the clac, and then let __{get,put}_user_nocheck() do their own thing?
> > or make it use the unsafe stuff?
> 
> Or.. we move the thing to assembly. Of course, I suck at (writing) asm,
> so the below is probably broken in various ways.

The advantage is that it now all lives in the same .o file and objtool
can actually follow and find the complete control flow.

I've made it ENDPROC() such that it becomes STT_FUNC and objtool does
all the normal things. I've also moved the ALIGN_DESTINATION macro into
the .S file.

Andy, do we have a sensible self-test for this path?

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Thu, Feb 28, 2019 at 04:22:04PM +0100, Dmitry Vyukov wrote:
> On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> > UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> > be called from there, move it into an exception much like BUG/WARN.
> >
> > *compile tested only*
> 
> 
> Please test it by booting KASAN kernel and then loading module
> produced by CONFIG_TEST_KASAN=y. There are too many subtle aspects to
> rely on "compile tested only", reviewers can't catch all of them
> either.

The below boots and survives test_kasan.

I'll now try that annotation you preferred. But I think this is a pretty
neat hack :-)

---
Subject: kasan,x86: Frob kasan_report() in an exception
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Feb 28 15:52:03 CET 2019

Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
UACCESS context, and kasan_report() is most definitely _NOT_ safe to
be called from there, move it into an exception much like BUg/WARN.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/bug.h   |   28 ++++++++++++++--------------
 arch/x86/include/asm/kasan.h |   17 +++++++++++++++++
 include/asm-generic/bug.h    |    1 +
 include/linux/kasan.h        |   12 +++++++++---
 lib/bug.c                    |    9 ++++++++-
 mm/kasan/kasan.h             |    2 +-
 mm/kasan/report.c            |    2 +-
 7 files changed, 51 insertions(+), 20 deletions(-)

--- 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[flag]"        "\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__),	\
+			 [flag] "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[flag]"   "\t# bug_entry::flags\n"	\
+		     "\t.org 2b+%c[size]\n"				\
 		     ".popsection"					\
-		     : : "i" (flags),					\
-			 "i" (sizeof(struct bug_entry)));		\
+		     : : [flag] "i" (flags),				\
+			 [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__);	\
 } while (0)
 
 #endif /* CONFIG_DEBUG_BUGVERBOSE */
--- a/arch/x86/include/asm/kasan.h
+++ b/arch/x86/include/asm/kasan.h
@@ -2,7 +2,10 @@
 #ifndef _ASM_X86_KASAN_H
 #define _ASM_X86_KASAN_H
 
+#include <asm/bug.h>
+
 #include <linux/const.h>
+#include <linux/sched.h>
 #define KASAN_SHADOW_OFFSET _AC(CONFIG_KASAN_SHADOW_OFFSET, UL)
 #define KASAN_SHADOW_SCALE_SHIFT 3
 
@@ -26,8 +29,22 @@
 #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)
+{
+	if (!current->kasan_depth) {
+		unsigned long rdi = addr, rsi = size, rdx = is_write, rcx = ip;
+		_BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN,
+				"D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
+		annotate_reachable();
+	}
+}
+#define kasan_report kasan_report
+
 #else
 static inline void kasan_early_init(void) { }
 static inline void kasan_init(void) { }
--- 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
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -83,6 +83,9 @@ size_t kasan_metadata_size(struct kmem_c
 bool kasan_save_enable_multi_shot(void);
 void kasan_restore_multi_shot(bool enabled);
 
+void __kasan_report(unsigned long addr, size_t size,
+		bool is_write, unsigned long ip);
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
@@ -153,8 +156,14 @@ static inline void kasan_remove_zero_sha
 static inline void kasan_unpoison_slab(const void *ptr) { }
 static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
 
+static inline void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) { }
+
 #endif /* CONFIG_KASAN */
 
+#ifndef kasan_report
+#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
+#endif
+
 #ifdef CONFIG_KASAN_GENERIC
 
 #define KASAN_SHADOW_INIT 0
@@ -177,9 +186,6 @@ void kasan_init_tags(void);
 
 void *kasan_reset_tag(const void *addr);
 
-void kasan_report(unsigned long addr, size_t size,
-		bool is_write, unsigned long ip);
-
 #else /* CONFIG_KASAN_SW_TAGS */
 
 static inline void kasan_init_tags(void) { }
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -47,6 +47,7 @@
 #include <linux/bug.h>
 #include <linux/sched.h>
 #include <linux/rculist.h>
+#include <linux/kasan.h>
 
 extern struct bug_entry __start___bug_table[], __stop___bug_table[];
 
@@ -144,7 +145,7 @@ enum bug_trap_type report_bug(unsigned l
 {
 	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 +168,7 @@ enum bug_trap_type report_bug(unsigned l
 		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 +190,11 @@ enum bug_trap_type report_bug(unsigned l
 		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)
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -130,7 +130,7 @@ void check_memory_region(unsigned long a
 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);
 
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *obj
 	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 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Fri, Mar 1, 2019 at 3:46 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Feb 28, 2019 at 04:22:04PM +0100, Dmitry Vyukov wrote:
> > On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> > > UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> > > be called from there, move it into an exception much like BUG/WARN.
> > >
> > > *compile tested only*
> >
> >
> > Please test it by booting KASAN kernel and then loading module
> > produced by CONFIG_TEST_KASAN=y. There are too many subtle aspects to
> > rely on "compile tested only", reviewers can't catch all of them
> > either.
>
> The below boots and survives test_kasan.
>
> I'll now try that annotation you preferred. But I think this is a pretty
> neat hack :-)

Yes, it's "neat" but it's also a "hack" :)

It involves asm, hardware exceptions, UD2 instructions. It also seems
to use arch-dependent code in arch-independent files: there is no RSI
on other arches, does this compile on non-x86? I understand you are
pretty comfortable with such code, but all else being equal normal C
code is preferable.
And it's not that we gain much due to this, we are merely working
around things. We tried to use UD2 for asan reports, but we emitted it
into the generated code where it had a chance of speeding up things
which could potentially justify hacks. But even there the final
decision was to go with normal calls.


> ---
> Subject: kasan,x86: Frob kasan_report() in an exception
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu Feb 28 15:52:03 CET 2019
>
> Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> be called from there, move it into an exception much like BUg/WARN.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/bug.h   |   28 ++++++++++++++--------------
>  arch/x86/include/asm/kasan.h |   17 +++++++++++++++++
>  include/asm-generic/bug.h    |    1 +
>  include/linux/kasan.h        |   12 +++++++++---
>  lib/bug.c                    |    9 ++++++++-
>  mm/kasan/kasan.h             |    2 +-
>  mm/kasan/report.c            |    2 +-
>  7 files changed, 51 insertions(+), 20 deletions(-)
>
> --- 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[flag]"        "\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__),  \
> +                        [flag] "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[flag]"   "\t# bug_entry::flags\n"      \
> +                    "\t.org 2b+%c[size]\n"                             \
>                      ".popsection"                                      \
> -                    : : "i" (flags),                                   \
> -                        "i" (sizeof(struct bug_entry)));               \
> +                    : : [flag] "i" (flags),                            \
> +                        [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__); \
>  } while (0)
>
>  #endif /* CONFIG_DEBUG_BUGVERBOSE */
> --- a/arch/x86/include/asm/kasan.h
> +++ b/arch/x86/include/asm/kasan.h
> @@ -2,7 +2,10 @@
>  #ifndef _ASM_X86_KASAN_H
>  #define _ASM_X86_KASAN_H
>
> +#include <asm/bug.h>
> +
>  #include <linux/const.h>
> +#include <linux/sched.h>
>  #define KASAN_SHADOW_OFFSET _AC(CONFIG_KASAN_SHADOW_OFFSET, UL)
>  #define KASAN_SHADOW_SCALE_SHIFT 3
>
> @@ -26,8 +29,22 @@
>  #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)
> +{
> +       if (!current->kasan_depth) {
> +               unsigned long rdi = addr, rsi = size, rdx = is_write, rcx = ip;
> +               _BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN,
> +                               "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
> +               annotate_reachable();
> +       }
> +}
> +#define kasan_report kasan_report
> +
>  #else
>  static inline void kasan_early_init(void) { }
>  static inline void kasan_init(void) { }
> --- 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
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -83,6 +83,9 @@ size_t kasan_metadata_size(struct kmem_c
>  bool kasan_save_enable_multi_shot(void);
>  void kasan_restore_multi_shot(bool enabled);
>
> +void __kasan_report(unsigned long addr, size_t size,
> +               bool is_write, unsigned long ip);
> +
>  #else /* CONFIG_KASAN */
>
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -153,8 +156,14 @@ static inline void kasan_remove_zero_sha
>  static inline void kasan_unpoison_slab(const void *ptr) { }
>  static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
>
> +static inline void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) { }
> +
>  #endif /* CONFIG_KASAN */
>
> +#ifndef kasan_report
> +#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
> +#endif
> +
>  #ifdef CONFIG_KASAN_GENERIC
>
>  #define KASAN_SHADOW_INIT 0
> @@ -177,9 +186,6 @@ void kasan_init_tags(void);
>
>  void *kasan_reset_tag(const void *addr);
>
> -void kasan_report(unsigned long addr, size_t size,
> -               bool is_write, unsigned long ip);
> -
>  #else /* CONFIG_KASAN_SW_TAGS */
>
>  static inline void kasan_init_tags(void) { }
> --- a/lib/bug.c
> +++ b/lib/bug.c
> @@ -47,6 +47,7 @@
>  #include <linux/bug.h>
>  #include <linux/sched.h>
>  #include <linux/rculist.h>
> +#include <linux/kasan.h>
>
>  extern struct bug_entry __start___bug_table[], __stop___bug_table[];
>
> @@ -144,7 +145,7 @@ enum bug_trap_type report_bug(unsigned l
>  {
>         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 +168,7 @@ enum bug_trap_type report_bug(unsigned l
>                 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 +190,11 @@ enum bug_trap_type report_bug(unsigned l
>                 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)
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -130,7 +130,7 @@ void check_memory_region(unsigned long a
>  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);
>
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *obj
>         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 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Fri, Mar 01, 2019 at 04:06:17PM +0100, Dmitry Vyukov wrote:
> It involves asm, hardware exceptions, UD2 instructions. It also seems
> to use arch-dependent code in arch-independent files: there is no RSI
> on other arches, does this compile on non-x86?

> > +#ifndef kasan_report
> > +#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
> > +#endif

Should build; but I've not tried yet.

I'll push the lot out to 0day, that'll yell loudly if I messed that up.

But yes, I'll try some annotation, see what that looks like.

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Andy Lutomirski]

> On Mar 1, 2019, at 6:38 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> On Fri, Mar 01, 2019 at 01:57:18PM +0100, Peter Zijlstra wrote:
>>> On Fri, Mar 01, 2019 at 01:27:45PM +0100, Peter Zijlstra wrote:
>>> arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x30: redundant UACCESS disable
>> 
>>> The usercopy one is difficult, that's copy_user_handle_tail(), it is
>>> buggered though, because that lacks notrace and thus has a __fentry__
>>> call in.
>>> 
>>> Also, afaict all exception jumps into copy_user_handle_tail() will have
>>> AC=1, but the __{get,put}_user_nocheck() things do STAC/CLAC all over
>>> again.
>>> 
>>> So what do we do? Annotate that we start with AC=1 and then immediately
>>> do the clac, and then let __{get,put}_user_nocheck() do their own thing?
>>> or make it use the unsafe stuff?
>> 
>> Or.. we move the thing to assembly. Of course, I suck at (writing) asm,
>> so the below is probably broken in various ways.
> 
> The advantage is that it now all lives in the same .o file and objtool
> can actually follow and find the complete control flow.
> 
> I've made it ENDPROC() such that it becomes STT_FUNC and objtool does
> all the normal things. I've also moved the ALIGN_DESTINATION macro into
> the .S file.
> 
> Andy, do we have a sensible self-test for this path?

Not that I know of. Something like my (rejected) strncpy_from_user test could probably be added fairly easily.

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Linus Torvalds]

On Fri, Mar 1, 2019 at 4:57 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Or.. we move the thing to assembly. Of course, I suck at (writing) asm,
> so the below is probably broken in various ways.

Looks sane, and makes sense. I feel that the old "mixed C and asm" was
much worse.

My only comment is that I think you should remove the ENTRY() one,
because you don't actually want that symbol to be global any more,
because I think it's all from inside copy_user_64.S now.

So just

     ALIGN
     copy_user_handle_tail:
          .. code goes here..

Hmm?

               Linus

Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC

[Linus Torvalds]

On Fri, Mar 1, 2019 at 2:35 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> With the dodgy patch:
>
> CC      drivers/gpu/drm/i915/i915_gem_execbuffer.o
> drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: eb_relocate_slow()+0x1f9: call to kvfree() with UACCESS enabled
> drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: i915_gem_execbuffer2_ioctl()+0x315: call to kvfree() with UACCESS enabled

Ok, so it looks like your patch does the right thing. Your fixups to
i915_gem_execbuffer.c (and the already commented-upon copy_user_tail
thing) also look sane.

               Linus

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Fri, Mar 01, 2019 at 04:23:05PM +0100, Peter Zijlstra wrote:

> But yes, I'll try some annotation, see what that looks like.

OK; that took a lot of time.. and a number of objtool bugs fixed but I
think I have something that I don't hate -- although it is not as solid
as I'd like it to be.


unmodified:

0000 0000000000000150 <__asan_load1>:
0000  150:      48 b8 ff ff ff ff ff    movabs $0xffff7fffffffffff,%rax
0007  157:      7f ff ff
000a  15a:      48 8b 0c 24             mov    (%rsp),%rcx
000e  15e:      48 39 c7                cmp    %rax,%rdi
0011  161:      76 23                   jbe    186 <__asan_load1+0x36>
0013  163:      48 b8 00 00 00 00 00    movabs $0xdffffc0000000000,%rax
001a  16a:      fc ff df
001d  16d:      48 89 fa                mov    %rdi,%rdx
0020  170:      48 c1 ea 03             shr    $0x3,%rdx
0024  174:      0f b6 04 02             movzbl (%rdx,%rax,1),%eax
0028  178:      84 c0                   test   %al,%al
002a  17a:      75 01                   jne    17d <__asan_load1+0x2d>
002c  17c:      c3                      retq
002d  17d:      89 fa                   mov    %edi,%edx
002f  17f:      83 e2 07                and    $0x7,%edx
0032  182:      38 d0                   cmp    %dl,%al
0034  184:      7f f6                   jg     17c <__asan_load1+0x2c>
0036  186:      31 d2                   xor    %edx,%edx
0038  188:      be 01 00 00 00          mov    $0x1,%esi
003d  18d:      e9 00 00 00 00          jmpq   192 <__asan_load1+0x42>
003e                    18e: R_X86_64_PLT32     kasan_report-0x4

exception:

0000 0000000000000150 <__asan_load1>:
0000  150:      48 b8 ff ff ff ff ff    movabs $0xffff7fffffffffff,%rax
0007  157:      7f ff ff
000a  15a:      48 8b 0c 24             mov    (%rsp),%rcx
000e  15e:      48 39 c7                cmp    %rax,%rdi
0011  161:      76 23                   jbe    186 <__asan_load1+0x36>
0013  163:      48 b8 00 00 00 00 00    movabs $0xdffffc0000000000,%rax
001a  16a:      fc ff df
001d  16d:      48 89 fa                mov    %rdi,%rdx
0020  170:      48 c1 ea 03             shr    $0x3,%rdx
0024  174:      0f b6 04 02             movzbl (%rdx,%rax,1),%eax
0028  178:      84 c0                   test   %al,%al
002a  17a:      75 01                   jne    17d <__asan_load1+0x2d>
002c  17c:      c3                      retq
002d  17d:      89 fa                   mov    %edi,%edx
002f  17f:      83 e2 07                and    $0x7,%edx
0032  182:      38 d0                   cmp    %dl,%al
0034  184:      7f f6                   jg     17c <__asan_load1+0x2c>
0036  186:      65 48 8b 04 25 00 00    mov    %gs:0x0,%rax
003d  18d:      00 00
003b                    18b: R_X86_64_32S       current_task
003f  18f:      8b 80 68 08 00 00       mov    0x868(%rax),%eax
0045  195:      85 c0                   test   %eax,%eax
0047  197:      75 e3                   jne    17c <__asan_load1+0x2c>
0049  199:      be 01 00 00 00          mov    $0x1,%esi
004e  19e:      31 d2                   xor    %edx,%edx
0050  1a0:      0f 0b                   ud2
0052  1a2:      c3                      retq

annotated:

0000 0000000000000150 <__asan_load1>:
0000  150:      48 b8 ff ff ff ff ff    movabs $0xffff7fffffffffff,%rax
0007  157:      7f ff ff
000a  15a:      53                      push   %rbx
000b  15b:      48 8b 4c 24 08          mov    0x8(%rsp),%rcx
0010  160:      48 39 c7                cmp    %rax,%rdi
0013  163:      76 24                   jbe    189 <__asan_load1+0x39>
0015  165:      48 b8 00 00 00 00 00    movabs $0xdffffc0000000000,%rax
001c  16c:      fc ff df
001f  16f:      48 89 fa                mov    %rdi,%rdx
0022  172:      48 c1 ea 03             shr    $0x3,%rdx
0026  176:      0f b6 04 02             movzbl (%rdx,%rax,1),%eax
002a  17a:      84 c0                   test   %al,%al
002c  17c:      75 02                   jne    180 <__asan_load1+0x30>
002e  17e:      5b                      pop    %rbx
002f  17f:      c3                      retq
0030  180:      89 fa                   mov    %edi,%edx
0032  182:      83 e2 07                and    $0x7,%edx
0035  185:      38 d0                   cmp    %dl,%al
0037  187:      7f f5                   jg     17e <__asan_load1+0x2e>
0039  189:      9c                      pushfq
003a  18a:      5b                      pop    %rbx
003b  18b:      90                      nop
003c  18c:      90                      nop
003d  18d:      90                      nop
003e  18e:      31 d2                   xor    %edx,%edx
0040  190:      be 01 00 00 00          mov    $0x1,%esi
0045  195:      e8 00 00 00 00          callq  19a <__asan_load1+0x4a>
0046                    196: R_X86_64_PLT32     __kasan_report-0x4
004a  19a:      53                      push   %rbx
004b  19b:      9d                      popfq
004c  19c:      5b                      pop    %rbx
004d  19d:      c3                      retq


---
--- a/arch/x86/include/asm/kasan.h
+++ b/arch/x86/include/asm/kasan.h
@@ -28,6 +28,23 @@
 #ifdef CONFIG_KASAN
 void __init kasan_early_init(void);
 void __init kasan_init(void);
+
+#include <asm/smap.h>
+
+extern void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip);
+
+static __always_inline
+void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
+{
+	unsigned long flags;
+
+	flags = smap_save();
+	__kasan_report(addr, size, is_write, ip);
+	smap_restore(flags);
+
+}
+#define kasan_report kasan_report
+
 #else
 static inline void kasan_early_init(void) { }
 static inline void kasan_init(void) { }
--- a/arch/x86/include/asm/smap.h
+++ b/arch/x86/include/asm/smap.h
@@ -46,6 +46,8 @@
 
 #ifdef CONFIG_X86_SMAP
 
+#include <asm/irqflags.h>
+
 static __always_inline void clac(void)
 {
 	/* Note: a barrier is implicit in alternative() */
@@ -58,6 +60,18 @@ static __always_inline void stac(void)
 	alternative("", __stringify(__ASM_STAC), X86_FEATURE_SMAP);
 }
 
+static __always_inline unsigned long smap_save(void)
+{
+	unsigned long flags = arch_local_save_flags();
+	clac();
+	return flags;
+}
+
+static __always_inline void smap_restore(unsigned long flags)
+{
+	arch_local_irq_restore(flags);
+}
+
 /* These macros can be used in asm() statements */
 #define ASM_CLAC \
 	ALTERNATIVE("", __stringify(__ASM_CLAC), X86_FEATURE_SMAP)
@@ -69,6 +83,9 @@ static __always_inline void stac(void)
 static inline void clac(void) { }
 static inline void stac(void) { }
 
+static inline unsigned long smap_save(void) { return 0; }
+static inline void smap_restore(unsigned long flags) { }
+
 #define ASM_CLAC
 #define ASM_STAC
 
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -83,6 +83,8 @@ size_t kasan_metadata_size(struct kmem_c
 bool kasan_save_enable_multi_shot(void);
 void kasan_restore_multi_shot(bool enabled);
 
+void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip);
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
@@ -153,8 +155,14 @@ static inline void kasan_remove_zero_sha
 static inline void kasan_unpoison_slab(const void *ptr) { }
 static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
 
+static inline void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) { }
+
 #endif /* CONFIG_KASAN */
 
+#ifndef kasan_report
+#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
+#endif
+
 #ifdef CONFIG_KASAN_GENERIC
 
 #define KASAN_SHADOW_INIT 0
@@ -177,9 +185,6 @@ void kasan_init_tags(void);
 
 void *kasan_reset_tag(const void *addr);
 
-void kasan_report(unsigned long addr, size_t size,
-		bool is_write, unsigned long ip);
-
 #else /* CONFIG_KASAN_SW_TAGS */
 
 static inline void kasan_init_tags(void) { }
--- a/mm/kasan/generic_report.c
+++ b/mm/kasan/generic_report.c
@@ -118,14 +118,14 @@ const char *get_bug_type(struct kasan_ac
 #define DEFINE_ASAN_REPORT_LOAD(size)                     \
 void __asan_report_load##size##_noabort(unsigned long addr) \
 {                                                         \
-	kasan_report(addr, size, false, _RET_IP_);	  \
+	__kasan_report(addr, size, false, _RET_IP_);	  \
 }                                                         \
 EXPORT_SYMBOL(__asan_report_load##size##_noabort)
 
 #define DEFINE_ASAN_REPORT_STORE(size)                     \
 void __asan_report_store##size##_noabort(unsigned long addr) \
 {                                                          \
-	kasan_report(addr, size, true, _RET_IP_);	   \
+	__kasan_report(addr, size, true, _RET_IP_);	   \
 }                                                          \
 EXPORT_SYMBOL(__asan_report_store##size##_noabort)
 
@@ -142,12 +142,12 @@ DEFINE_ASAN_REPORT_STORE(16);
 
 void __asan_report_load_n_noabort(unsigned long addr, size_t size)
 {
-	kasan_report(addr, size, false, _RET_IP_);
+	__kasan_report(addr, size, false, _RET_IP_);
 }
 EXPORT_SYMBOL(__asan_report_load_n_noabort);
 
 void __asan_report_store_n_noabort(unsigned long addr, size_t size)
 {
-	kasan_report(addr, size, true, _RET_IP_);
+	__kasan_report(addr, size, true, _RET_IP_);
 }
 EXPORT_SYMBOL(__asan_report_store_n_noabort);
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -130,8 +130,6 @@ void check_memory_region(unsigned long a
 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,
-		bool is_write, unsigned long ip);
 void kasan_report_invalid_free(void *object, unsigned long ip);
 
 #if defined(CONFIG_KASAN_GENERIC) && \
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *obj
 	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;
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -43,6 +43,7 @@ enum op_dest_type {
 	OP_DEST_REG_INDIRECT,
 	OP_DEST_MEM,
 	OP_DEST_PUSH,
+	OP_DEST_PUSHF,
 	OP_DEST_LEAVE,
 };
 
@@ -57,6 +58,7 @@ enum op_src_type {
 	OP_SRC_REG_INDIRECT,
 	OP_SRC_CONST,
 	OP_SRC_POP,
+	OP_SRC_POPF,
 	OP_SRC_ADD,
 	OP_SRC_AND,
 };
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -357,13 +357,13 @@ int arch_decode_instruction(struct elf *
 		/* pushf */
 		*type = INSN_STACK;
 		op->src.type = OP_SRC_CONST;
-		op->dest.type = OP_DEST_PUSH;
+		op->dest.type = OP_DEST_PUSHF;
 		break;
 
 	case 0x9d:
 		/* popf */
 		*type = INSN_STACK;
-		op->src.type = OP_SRC_POP;
+		op->src.type = OP_SRC_POPF;
 		op->dest.type = OP_DEST_MEM;
 		break;
 
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1359,11 +1359,11 @@ static int update_insn_state_regs(struct
 		return 0;
 
 	/* push */
-	if (op->dest.type == OP_DEST_PUSH)
+	if (op->dest.type == OP_DEST_PUSH || op->dest.type == OP_DEST_PUSHF)
 		cfa->offset += 8;
 
 	/* pop */
-	if (op->src.type == OP_SRC_POP)
+	if (op->src.type == OP_SRC_POP || op->src.type == OP_SRC_POPF)
 		cfa->offset -= 8;
 
 	/* add immediate to sp */
@@ -1620,6 +1620,7 @@ static int update_insn_state(struct inst
 			break;
 
 		case OP_SRC_POP:
+		case OP_SRC_POPF:
 			if (!state->drap && op->dest.type == OP_DEST_REG &&
 			    op->dest.reg == cfa->base) {
 
@@ -1684,6 +1685,7 @@ static int update_insn_state(struct inst
 		break;
 
 	case OP_DEST_PUSH:
+	case OP_DEST_PUSHF:
 		state->stack_size += 8;
 		if (cfa->base == CFI_SP)
 			cfa->offset += 8;
@@ -1774,7 +1776,7 @@ static int update_insn_state(struct inst
 		break;
 
 	case OP_DEST_MEM:
-		if (op->src.type != OP_SRC_POP) {
+		if (op->src.type != OP_SRC_POP && op->src.type != OP_SRC_POPF) {
 			WARN_FUNC("unknown stack-related memory operation",
 				  insn->sec, insn->offset);
 			return -1;
@@ -2071,6 +2073,16 @@ static int validate_branch(struct objtoo
 			if (update_insn_state(insn, &state))
 				return 1;
 
+			if (insn->stack_op.dest.type == OP_DEST_PUSHF) {
+				if (state.uaccess)
+					state.uaccess_stack++;
+			}
+
+			if (insn->stack_op.src.type == OP_SRC_POPF) {
+				if (state.uaccess_stack && !--state.uaccess_stack)
+					state.uaccess = func_uaccess_safe(func);
+			}
+
 			break;
 
 		case INSN_STAC:
@@ -2088,7 +2100,7 @@ static int validate_branch(struct objtoo
 				return 1;
 			}
 
-			if (func_uaccess_safe(func)) {
+			if (func_uaccess_safe(func) && !state.uaccess_stack) {
 				WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
 				return 1;
 			}
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -32,6 +32,7 @@ struct insn_state {
 	unsigned char type;
 	bool bp_scratch;
 	bool drap, end, uaccess;
+	int uaccess_stack;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];
 };

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Wed, Mar 6, 2019 at 2:13 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Mar 01, 2019 at 04:23:05PM +0100, Peter Zijlstra wrote:
>
> > But yes, I'll try some annotation, see what that looks like.
>
> OK; that took a lot of time.. and a number of objtool bugs fixed but I
> think I have something that I don't hate -- although it is not as solid
> as I'd like it to be.
>
>
> unmodified:
>
> 0000 0000000000000150 <__asan_load1>:
> 0000  150:      48 b8 ff ff ff ff ff    movabs $0xffff7fffffffffff,%rax
> 0007  157:      7f ff ff
> 000a  15a:      48 8b 0c 24             mov    (%rsp),%rcx
> 000e  15e:      48 39 c7                cmp    %rax,%rdi
> 0011  161:      76 23                   jbe    186 <__asan_load1+0x36>
> 0013  163:      48 b8 00 00 00 00 00    movabs $0xdffffc0000000000,%rax
> 001a  16a:      fc ff df
> 001d  16d:      48 89 fa                mov    %rdi,%rdx
> 0020  170:      48 c1 ea 03             shr    $0x3,%rdx
> 0024  174:      0f b6 04 02             movzbl (%rdx,%rax,1),%eax
> 0028  178:      84 c0                   test   %al,%al
> 002a  17a:      75 01                   jne    17d <__asan_load1+0x2d>
> 002c  17c:      c3                      retq
> 002d  17d:      89 fa                   mov    %edi,%edx
> 002f  17f:      83 e2 07                and    $0x7,%edx
> 0032  182:      38 d0                   cmp    %dl,%al
> 0034  184:      7f f6                   jg     17c <__asan_load1+0x2c>
> 0036  186:      31 d2                   xor    %edx,%edx
> 0038  188:      be 01 00 00 00          mov    $0x1,%esi
> 003d  18d:      e9 00 00 00 00          jmpq   192 <__asan_load1+0x42>
> 003e                    18e: R_X86_64_PLT32     kasan_report-0x4
>
> exception:
>
> 0000 0000000000000150 <__asan_load1>:
> 0000  150:      48 b8 ff ff ff ff ff    movabs $0xffff7fffffffffff,%rax
> 0007  157:      7f ff ff
> 000a  15a:      48 8b 0c 24             mov    (%rsp),%rcx
> 000e  15e:      48 39 c7                cmp    %rax,%rdi
> 0011  161:      76 23                   jbe    186 <__asan_load1+0x36>
> 0013  163:      48 b8 00 00 00 00 00    movabs $0xdffffc0000000000,%rax
> 001a  16a:      fc ff df
> 001d  16d:      48 89 fa                mov    %rdi,%rdx
> 0020  170:      48 c1 ea 03             shr    $0x3,%rdx
> 0024  174:      0f b6 04 02             movzbl (%rdx,%rax,1),%eax
> 0028  178:      84 c0                   test   %al,%al
> 002a  17a:      75 01                   jne    17d <__asan_load1+0x2d>
> 002c  17c:      c3                      retq
> 002d  17d:      89 fa                   mov    %edi,%edx
> 002f  17f:      83 e2 07                and    $0x7,%edx
> 0032  182:      38 d0                   cmp    %dl,%al
> 0034  184:      7f f6                   jg     17c <__asan_load1+0x2c>
> 0036  186:      65 48 8b 04 25 00 00    mov    %gs:0x0,%rax
> 003d  18d:      00 00
> 003b                    18b: R_X86_64_32S       current_task
> 003f  18f:      8b 80 68 08 00 00       mov    0x868(%rax),%eax
> 0045  195:      85 c0                   test   %eax,%eax
> 0047  197:      75 e3                   jne    17c <__asan_load1+0x2c>
> 0049  199:      be 01 00 00 00          mov    $0x1,%esi
> 004e  19e:      31 d2                   xor    %edx,%edx
> 0050  1a0:      0f 0b                   ud2
> 0052  1a2:      c3                      retq
>
> annotated:
>
> 0000 0000000000000150 <__asan_load1>:
> 0000  150:      48 b8 ff ff ff ff ff    movabs $0xffff7fffffffffff,%rax
> 0007  157:      7f ff ff
> 000a  15a:      53                      push   %rbx

/\/\/\/\/\/\

This push is unpleasant on hot fast path. I think we need to move
whole report cold path into a separate noinline function as it is now,
and that function will do the magic with smap. Then this won't prevent
tail calling and won't affect fast-path codegen.

> 000b  15b:      48 8b 4c 24 08          mov    0x8(%rsp),%rcx
> 0010  160:      48 39 c7                cmp    %rax,%rdi
> 0013  163:      76 24                   jbe    189 <__asan_load1+0x39>
> 0015  165:      48 b8 00 00 00 00 00    movabs $0xdffffc0000000000,%rax
> 001c  16c:      fc ff df
> 001f  16f:      48 89 fa                mov    %rdi,%rdx
> 0022  172:      48 c1 ea 03             shr    $0x3,%rdx
> 0026  176:      0f b6 04 02             movzbl (%rdx,%rax,1),%eax
> 002a  17a:      84 c0                   test   %al,%al
> 002c  17c:      75 02                   jne    180 <__asan_load1+0x30>
> 002e  17e:      5b                      pop    %rbx
> 002f  17f:      c3                      retq
> 0030  180:      89 fa                   mov    %edi,%edx
> 0032  182:      83 e2 07                and    $0x7,%edx
> 0035  185:      38 d0                   cmp    %dl,%al
> 0037  187:      7f f5                   jg     17e <__asan_load1+0x2e>
> 0039  189:      9c                      pushfq
> 003a  18a:      5b                      pop    %rbx
> 003b  18b:      90                      nop
> 003c  18c:      90                      nop
> 003d  18d:      90                      nop
> 003e  18e:      31 d2                   xor    %edx,%edx
> 0040  190:      be 01 00 00 00          mov    $0x1,%esi
> 0045  195:      e8 00 00 00 00          callq  19a <__asan_load1+0x4a>
> 0046                    196: R_X86_64_PLT32     __kasan_report-0x4
> 004a  19a:      53                      push   %rbx
> 004b  19b:      9d                      popfq
> 004c  19c:      5b                      pop    %rbx
> 004d  19d:      c3                      retq
>
>
> ---
> --- a/arch/x86/include/asm/kasan.h
> +++ b/arch/x86/include/asm/kasan.h
> @@ -28,6 +28,23 @@
>  #ifdef CONFIG_KASAN
>  void __init kasan_early_init(void);
>  void __init kasan_init(void);
> +
> +#include <asm/smap.h>
> +
> +extern void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip);
> +
> +static __always_inline
> +void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
> +{
> +       unsigned long flags;
> +
> +       flags = smap_save();

Previously you said that messing with smap here causes boot errors.
Shouldn't we do smap_save iff kasan_report_enabled? Otherwise we just
bail out, so no need to enable/disable smap.

> +       __kasan_report(addr, size, is_write, ip);
> +       smap_restore(flags);
> +
> +}
> +#define kasan_report kasan_report
> +
>  #else
>  static inline void kasan_early_init(void) { }
>  static inline void kasan_init(void) { }
> --- a/arch/x86/include/asm/smap.h
> +++ b/arch/x86/include/asm/smap.h
> @@ -46,6 +46,8 @@
>
>  #ifdef CONFIG_X86_SMAP
>
> +#include <asm/irqflags.h>
> +
>  static __always_inline void clac(void)
>  {
>         /* Note: a barrier is implicit in alternative() */
> @@ -58,6 +60,18 @@ static __always_inline void stac(void)
>         alternative("", __stringify(__ASM_STAC), X86_FEATURE_SMAP);
>  }
>
> +static __always_inline unsigned long smap_save(void)
> +{
> +       unsigned long flags = arch_local_save_flags();
> +       clac();
> +       return flags;
> +}
> +
> +static __always_inline void smap_restore(unsigned long flags)
> +{
> +       arch_local_irq_restore(flags);
> +}
> +
>  /* These macros can be used in asm() statements */
>  #define ASM_CLAC \
>         ALTERNATIVE("", __stringify(__ASM_CLAC), X86_FEATURE_SMAP)
> @@ -69,6 +83,9 @@ static __always_inline void stac(void)
>  static inline void clac(void) { }
>  static inline void stac(void) { }
>
> +static inline unsigned long smap_save(void) { return 0; }
> +static inline void smap_restore(unsigned long flags) { }
> +
>  #define ASM_CLAC
>  #define ASM_STAC
>
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -83,6 +83,8 @@ size_t kasan_metadata_size(struct kmem_c
>  bool kasan_save_enable_multi_shot(void);
>  void kasan_restore_multi_shot(bool enabled);
>
> +void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip);
> +
>  #else /* CONFIG_KASAN */
>
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -153,8 +155,14 @@ static inline void kasan_remove_zero_sha
>  static inline void kasan_unpoison_slab(const void *ptr) { }
>  static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
>
> +static inline void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) { }
> +
>  #endif /* CONFIG_KASAN */
>
> +#ifndef kasan_report
> +#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
> +#endif
> +
>  #ifdef CONFIG_KASAN_GENERIC
>
>  #define KASAN_SHADOW_INIT 0
> @@ -177,9 +185,6 @@ void kasan_init_tags(void);
>
>  void *kasan_reset_tag(const void *addr);
>
> -void kasan_report(unsigned long addr, size_t size,
> -               bool is_write, unsigned long ip);
> -
>  #else /* CONFIG_KASAN_SW_TAGS */
>
>  static inline void kasan_init_tags(void) { }
> --- a/mm/kasan/generic_report.c
> +++ b/mm/kasan/generic_report.c
> @@ -118,14 +118,14 @@ const char *get_bug_type(struct kasan_ac
>  #define DEFINE_ASAN_REPORT_LOAD(size)                     \
>  void __asan_report_load##size##_noabort(unsigned long addr) \
>  {                                                         \
> -       kasan_report(addr, size, false, _RET_IP_);        \
> +       __kasan_report(addr, size, false, _RET_IP_);      \

Unless I am missing something, this seems to make this patch no-op. We
fixed kasan_report for smap, but here we now use __kasan_report which
is not fixed. So this won't work with smap again?..


>  }                                                         \
>  EXPORT_SYMBOL(__asan_report_load##size##_noabort)
>
>  #define DEFINE_ASAN_REPORT_STORE(size)                     \
>  void __asan_report_store##size##_noabort(unsigned long addr) \
>  {                                                          \
> -       kasan_report(addr, size, true, _RET_IP_);          \
> +       __kasan_report(addr, size, true, _RET_IP_);        \
>  }                                                          \
>  EXPORT_SYMBOL(__asan_report_store##size##_noabort)
>
> @@ -142,12 +142,12 @@ DEFINE_ASAN_REPORT_STORE(16);
>
>  void __asan_report_load_n_noabort(unsigned long addr, size_t size)
>  {
> -       kasan_report(addr, size, false, _RET_IP_);
> +       __kasan_report(addr, size, false, _RET_IP_);
>  }
>  EXPORT_SYMBOL(__asan_report_load_n_noabort);
>
>  void __asan_report_store_n_noabort(unsigned long addr, size_t size)
>  {
> -       kasan_report(addr, size, true, _RET_IP_);
> +       __kasan_report(addr, size, true, _RET_IP_);
>  }
>  EXPORT_SYMBOL(__asan_report_store_n_noabort);
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -130,8 +130,6 @@ void check_memory_region(unsigned long a
>  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,
> -               bool is_write, unsigned long ip);
>  void kasan_report_invalid_free(void *object, unsigned long ip);
>
>  #if defined(CONFIG_KASAN_GENERIC) && \
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *obj
>         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;
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -43,6 +43,7 @@ enum op_dest_type {
>         OP_DEST_REG_INDIRECT,
>         OP_DEST_MEM,
>         OP_DEST_PUSH,
> +       OP_DEST_PUSHF,
>         OP_DEST_LEAVE,
>  };
>
> @@ -57,6 +58,7 @@ enum op_src_type {
>         OP_SRC_REG_INDIRECT,
>         OP_SRC_CONST,
>         OP_SRC_POP,
> +       OP_SRC_POPF,
>         OP_SRC_ADD,
>         OP_SRC_AND,
>  };
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -357,13 +357,13 @@ int arch_decode_instruction(struct elf *
>                 /* pushf */
>                 *type = INSN_STACK;
>                 op->src.type = OP_SRC_CONST;
> -               op->dest.type = OP_DEST_PUSH;
> +               op->dest.type = OP_DEST_PUSHF;
>                 break;
>
>         case 0x9d:
>                 /* popf */
>                 *type = INSN_STACK;
> -               op->src.type = OP_SRC_POP;
> +               op->src.type = OP_SRC_POPF;
>                 op->dest.type = OP_DEST_MEM;
>                 break;
>
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1359,11 +1359,11 @@ static int update_insn_state_regs(struct
>                 return 0;
>
>         /* push */
> -       if (op->dest.type == OP_DEST_PUSH)
> +       if (op->dest.type == OP_DEST_PUSH || op->dest.type == OP_DEST_PUSHF)
>                 cfa->offset += 8;
>
>         /* pop */
> -       if (op->src.type == OP_SRC_POP)
> +       if (op->src.type == OP_SRC_POP || op->src.type == OP_SRC_POPF)
>                 cfa->offset -= 8;
>
>         /* add immediate to sp */
> @@ -1620,6 +1620,7 @@ static int update_insn_state(struct inst
>                         break;
>
>                 case OP_SRC_POP:
> +               case OP_SRC_POPF:
>                         if (!state->drap && op->dest.type == OP_DEST_REG &&
>                             op->dest.reg == cfa->base) {
>
> @@ -1684,6 +1685,7 @@ static int update_insn_state(struct inst
>                 break;
>
>         case OP_DEST_PUSH:
> +       case OP_DEST_PUSHF:
>                 state->stack_size += 8;
>                 if (cfa->base == CFI_SP)
>                         cfa->offset += 8;
> @@ -1774,7 +1776,7 @@ static int update_insn_state(struct inst
>                 break;
>
>         case OP_DEST_MEM:
> -               if (op->src.type != OP_SRC_POP) {
> +               if (op->src.type != OP_SRC_POP && op->src.type != OP_SRC_POPF) {
>                         WARN_FUNC("unknown stack-related memory operation",
>                                   insn->sec, insn->offset);
>                         return -1;
> @@ -2071,6 +2073,16 @@ static int validate_branch(struct objtoo
>                         if (update_insn_state(insn, &state))
>                                 return 1;
>
> +                       if (insn->stack_op.dest.type == OP_DEST_PUSHF) {
> +                               if (state.uaccess)
> +                                       state.uaccess_stack++;
> +                       }
> +
> +                       if (insn->stack_op.src.type == OP_SRC_POPF) {
> +                               if (state.uaccess_stack && !--state.uaccess_stack)
> +                                       state.uaccess = func_uaccess_safe(func);
> +                       }
> +
>                         break;
>
>                 case INSN_STAC:
> @@ -2088,7 +2100,7 @@ static int validate_branch(struct objtoo
>                                 return 1;
>                         }
>
> -                       if (func_uaccess_safe(func)) {
> +                       if (func_uaccess_safe(func) && !state.uaccess_stack) {
>                                 WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
>                                 return 1;
>                         }
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -32,6 +32,7 @@ struct insn_state {
>         unsigned char type;
>         bool bp_scratch;
>         bool drap, end, uaccess;
> +       int uaccess_stack;
>         int drap_reg, drap_offset;
>         struct cfi_reg vals[CFI_NUM_REGS];
>  };

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Wed, Mar 06, 2019 at 02:39:33PM +0100, Dmitry Vyukov wrote:
> On Wed, Mar 6, 2019 at 2:13 PM Peter Zijlstra <peterz@infradead.org> wrote:

> > annotated:
> >
> > 0000 0000000000000150 <__asan_load1>:
> > 0000  150:      48 b8 ff ff ff ff ff    movabs $0xffff7fffffffffff,%rax
> > 0007  157:      7f ff ff
> > 000a  15a:      53                      push   %rbx
> 
> /\/\/\/\/\/\
> 
> This push is unpleasant on hot fast path. I think we need to move
> whole report cold path into a separate noinline function as it is now,
> and that function will do the magic with smap. Then this won't prevent
> tail calling and won't affect fast-path codegen.

It's a bit daft of GCC to do that anyway; since it only uses that rbx
thing in the cold path at __asan_load1+0x30.

But yes, that wants fixing or something. Then again; a kernel with KASAN
on is unbearable slow anyway.

> > 000b  15b:      48 8b 4c 24 08          mov    0x8(%rsp),%rcx
> > 0010  160:      48 39 c7                cmp    %rax,%rdi
> > 0013  163:      76 24                   jbe    189 <__asan_load1+0x39>
> > 0015  165:      48 b8 00 00 00 00 00    movabs $0xdffffc0000000000,%rax
> > 001c  16c:      fc ff df
> > 001f  16f:      48 89 fa                mov    %rdi,%rdx
> > 0022  172:      48 c1 ea 03             shr    $0x3,%rdx
> > 0026  176:      0f b6 04 02             movzbl (%rdx,%rax,1),%eax
> > 002a  17a:      84 c0                   test   %al,%al
> > 002c  17c:      75 02                   jne    180 <__asan_load1+0x30>
> > 002e  17e:      5b                      pop    %rbx
> > 002f  17f:      c3                      retq

^^^ hot path, vvv cold path

> > 0030  180:      89 fa                   mov    %edi,%edx
> > 0032  182:      83 e2 07                and    $0x7,%edx
> > 0035  185:      38 d0                   cmp    %dl,%al
> > 0037  187:      7f f5                   jg     17e <__asan_load1+0x2e>
> > 0039  189:      9c                      pushfq
> > 003a  18a:      5b                      pop    %rbx
> > 003b  18b:      90                      nop
> > 003c  18c:      90                      nop
> > 003d  18d:      90                      nop
> > 003e  18e:      31 d2                   xor    %edx,%edx
> > 0040  190:      be 01 00 00 00          mov    $0x1,%esi
> > 0045  195:      e8 00 00 00 00          callq  19a <__asan_load1+0x4a>
> > 0046                    196: R_X86_64_PLT32     __kasan_report-0x4
> > 004a  19a:      53                      push   %rbx
> > 004b  19b:      9d                      popfq
> > 004c  19c:      5b                      pop    %rbx
> > 004d  19d:      c3                      retq

> > +static __always_inline
> > +void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
> > +{
> > +       unsigned long flags;
> > +
> > +       flags = smap_save();
> 
> Previously you said that messing with smap here causes boot errors.
> Shouldn't we do smap_save iff kasan_report_enabled? Otherwise we just
> bail out, so no need to enable/disable smap.
> 
> > +       __kasan_report(addr, size, is_write, ip);
> > +       smap_restore(flags);
> > +
> > +}

Ah, you think I booted this :-) Still, this is only PUSHF;CLAC, which I
think should actually work really early. It was that #UD thing that
didn't work early, simply because we'd not set up the exception vector
yet when first this happens.

> > --- a/mm/kasan/generic_report.c
> > +++ b/mm/kasan/generic_report.c
> > @@ -118,14 +118,14 @@ const char *get_bug_type(struct kasan_ac
> >  #define DEFINE_ASAN_REPORT_LOAD(size)                     \
> >  void __asan_report_load##size##_noabort(unsigned long addr) \
> >  {                                                         \
> > -       kasan_report(addr, size, false, _RET_IP_);        \
> > +       __kasan_report(addr, size, false, _RET_IP_);      \
> 
> Unless I am missing something, this seems to make this patch no-op. We
> fixed kasan_report for smap, but here we now use __kasan_report which
> is not fixed. So this won't work with smap again?..

I've not found callers of __asan_report_load* with AC=1 in the kernel
yet. Under what condtions does GCC emit calls to these functions?

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Wed, Mar 6, 2019 at 2:57 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Mar 06, 2019 at 02:39:33PM +0100, Dmitry Vyukov wrote:
> > On Wed, Mar 6, 2019 at 2:13 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > annotated:
> > >
> > > 0000 0000000000000150 <__asan_load1>:
> > > 0000  150:      48 b8 ff ff ff ff ff    movabs $0xffff7fffffffffff,%rax
> > > 0007  157:      7f ff ff
> > > 000a  15a:      53                      push   %rbx
> >
> > /\/\/\/\/\/\
> >
> > This push is unpleasant on hot fast path. I think we need to move
> > whole report cold path into a separate noinline function as it is now,
> > and that function will do the magic with smap. Then this won't prevent
> > tail calling and won't affect fast-path codegen.
>
> It's a bit daft of GCC to do that anyway; since it only uses that rbx
> thing in the cold path at __asan_load1+0x30.
>
> But yes, that wants fixing or something. Then again; a kernel with KASAN
> on is unbearable slow anyway.
>
> > > 000b  15b:      48 8b 4c 24 08          mov    0x8(%rsp),%rcx
> > > 0010  160:      48 39 c7                cmp    %rax,%rdi
> > > 0013  163:      76 24                   jbe    189 <__asan_load1+0x39>
> > > 0015  165:      48 b8 00 00 00 00 00    movabs $0xdffffc0000000000,%rax
> > > 001c  16c:      fc ff df
> > > 001f  16f:      48 89 fa                mov    %rdi,%rdx
> > > 0022  172:      48 c1 ea 03             shr    $0x3,%rdx
> > > 0026  176:      0f b6 04 02             movzbl (%rdx,%rax,1),%eax
> > > 002a  17a:      84 c0                   test   %al,%al
> > > 002c  17c:      75 02                   jne    180 <__asan_load1+0x30>
> > > 002e  17e:      5b                      pop    %rbx
> > > 002f  17f:      c3                      retq
>
> ^^^ hot path, vvv cold path
>
> > > 0030  180:      89 fa                   mov    %edi,%edx
> > > 0032  182:      83 e2 07                and    $0x7,%edx
> > > 0035  185:      38 d0                   cmp    %dl,%al
> > > 0037  187:      7f f5                   jg     17e <__asan_load1+0x2e>
> > > 0039  189:      9c                      pushfq
> > > 003a  18a:      5b                      pop    %rbx
> > > 003b  18b:      90                      nop
> > > 003c  18c:      90                      nop
> > > 003d  18d:      90                      nop
> > > 003e  18e:      31 d2                   xor    %edx,%edx
> > > 0040  190:      be 01 00 00 00          mov    $0x1,%esi
> > > 0045  195:      e8 00 00 00 00          callq  19a <__asan_load1+0x4a>
> > > 0046                    196: R_X86_64_PLT32     __kasan_report-0x4
> > > 004a  19a:      53                      push   %rbx
> > > 004b  19b:      9d                      popfq
> > > 004c  19c:      5b                      pop    %rbx
> > > 004d  19d:      c3                      retq
>
> > > +static __always_inline
> > > +void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
> > > +{
> > > +       unsigned long flags;
> > > +
> > > +       flags = smap_save();
> >
> > Previously you said that messing with smap here causes boot errors.
> > Shouldn't we do smap_save iff kasan_report_enabled? Otherwise we just
> > bail out, so no need to enable/disable smap.
> >
> > > +       __kasan_report(addr, size, is_write, ip);
> > > +       smap_restore(flags);
> > > +
> > > +}
>
> Ah, you think I booted this :-) Still, this is only PUSHF;CLAC, which I
> think should actually work really early. It was that #UD thing that
> didn't work early, simply because we'd not set up the exception vector
> yet when first this happens.
>
> > > --- a/mm/kasan/generic_report.c
> > > +++ b/mm/kasan/generic_report.c
> > > @@ -118,14 +118,14 @@ const char *get_bug_type(struct kasan_ac
> > >  #define DEFINE_ASAN_REPORT_LOAD(size)                     \
> > >  void __asan_report_load##size##_noabort(unsigned long addr) \
> > >  {                                                         \
> > > -       kasan_report(addr, size, false, _RET_IP_);        \
> > > +       __kasan_report(addr, size, false, _RET_IP_);      \
> >
> > Unless I am missing something, this seems to make this patch no-op. We
> > fixed kasan_report for smap, but here we now use __kasan_report which
> > is not fixed. So this won't work with smap again?..
>
> I've not found callers of __asan_report_load* with AC=1 in the kernel
> yet. Under what condtions does GCC emit calls to these functions?

CONFIG_KASAN_INLINE=y
Then compiler inlines fast path into generated code and only calls
into runtime to report errors (also, faster, this should be a default
for anything other than tiny ROM controllers).

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Wed, Mar 06, 2019 at 03:01:33PM +0100, Dmitry Vyukov wrote:
> On Wed, Mar 6, 2019 at 2:57 PM Peter Zijlstra <peterz@infradead.org> wrote:

> > I've not found callers of __asan_report_load* with AC=1 in the kernel
> > yet. Under what condtions does GCC emit calls to these functions?
> 
> CONFIG_KASAN_INLINE=y
> Then compiler inlines fast path into generated code and only calls
> into runtime to report errors (also, faster, this should be a default
> for anything other than tiny ROM controllers).

*sigh*, clearly I've not build enough kernels yet... Lemme go try that.

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Wed, Mar 06, 2019 at 03:12:37PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 06, 2019 at 03:01:33PM +0100, Dmitry Vyukov wrote:
> > On Wed, Mar 6, 2019 at 2:57 PM Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > I've not found callers of __asan_report_load* with AC=1 in the kernel
> > > yet. Under what condtions does GCC emit calls to these functions?
> > 
> > CONFIG_KASAN_INLINE=y
> > Then compiler inlines fast path into generated code and only calls
> > into runtime to report errors (also, faster, this should be a default
> > for anything other than tiny ROM controllers).
> 
> *sigh*, clearly I've not build enough kernels yet... Lemme go try that.

mm/kasan/generic_report.o: warning: objtool: __asan_report_load1_noabort()+0x0: call to __fentry__() with UACCESS enabled

You want to do:

CFLAGS_REMOVE_generic_report.o = -pg

like generic.o has?

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Wed, Mar 6, 2019 at 3:34 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Mar 06, 2019 at 03:12:37PM +0100, Peter Zijlstra wrote:
> > On Wed, Mar 06, 2019 at 03:01:33PM +0100, Dmitry Vyukov wrote:
> > > On Wed, Mar 6, 2019 at 2:57 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > > I've not found callers of __asan_report_load* with AC=1 in the kernel
> > > > yet. Under what condtions does GCC emit calls to these functions?
> > >
> > > CONFIG_KASAN_INLINE=y
> > > Then compiler inlines fast path into generated code and only calls
> > > into runtime to report errors (also, faster, this should be a default
> > > for anything other than tiny ROM controllers).
> >
> > *sigh*, clearly I've not build enough kernels yet... Lemme go try that.
>
> mm/kasan/generic_report.o: warning: objtool: __asan_report_load1_noabort()+0x0: call to __fentry__() with UACCESS enabled
>
> You want to do:
>
> CFLAGS_REMOVE_f= -pg
>
> like generic.o has?

This should not matter for KASAN itself.
KASAN will call into function tracer, and function tracer will call
into KASAN, but unless function tracer is badly broken and causes a
KASAN report on every invocation, the recursion will end (function
tracer will get to the _report_ function). So we only disabled -pg for
fast paths.
But if it makes things simpler for the objtool, then I think we can
disable function tracer for generic_report.c too.

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Wed, Mar 6, 2019 at 3:40 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Wed, Mar 6, 2019 at 3:34 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Mar 06, 2019 at 03:12:37PM +0100, Peter Zijlstra wrote:
> > > On Wed, Mar 06, 2019 at 03:01:33PM +0100, Dmitry Vyukov wrote:
> > > > On Wed, Mar 6, 2019 at 2:57 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > > > I've not found callers of __asan_report_load* with AC=1 in the kernel
> > > > > yet. Under what condtions does GCC emit calls to these functions?
> > > >
> > > > CONFIG_KASAN_INLINE=y
> > > > Then compiler inlines fast path into generated code and only calls
> > > > into runtime to report errors (also, faster, this should be a default
> > > > for anything other than tiny ROM controllers).
> > >
> > > *sigh*, clearly I've not build enough kernels yet... Lemme go try that.
> >
> > mm/kasan/generic_report.o: warning: objtool: __asan_report_load1_noabort()+0x0: call to __fentry__() with UACCESS enabled
> >
> > You want to do:
> >
> > CFLAGS_REMOVE_f= -pg
> >
> > like generic.o has?
>
> This should not matter for KASAN itself.
> KASAN will call into function tracer, and function tracer will call
> into KASAN, but unless function tracer is badly broken and causes a
> KASAN report on every invocation, the recursion will end (function
> tracer will get to the _report_ function). So we only disabled -pg for

tracer will _not_ get to the _report_ function

> fast paths.
> But if it makes things simpler for the objtool, then I think we can
> disable function tracer for generic_report.c too.

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Wed, Mar 06, 2019 at 03:40:51PM +0100, Dmitry Vyukov wrote:
> On Wed, Mar 6, 2019 at 3:34 PM Peter Zijlstra <peterz@infradead.org> wrote:

> > mm/kasan/generic_report.o: warning: objtool: __asan_report_load1_noabort()+0x0: call to __fentry__() with UACCESS enabled

> But if it makes things simpler for the objtool, then I think we can
> disable function tracer for generic_report.c too.

It's not simpler; it is actually a correctness issue. Those functions
must not call into the tracer with AC=1

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov]

On Wed, Mar 6, 2019 at 3:56 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Mar 06, 2019 at 03:40:51PM +0100, Dmitry Vyukov wrote:
> > On Wed, Mar 6, 2019 at 3:34 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > mm/kasan/generic_report.o: warning: objtool: __asan_report_load1_noabort()+0x0: call to __fentry__() with UACCESS enabled
>
> > But if it makes things simpler for the objtool, then I think we can
> > disable function tracer for generic_report.c too.
>
> It's not simpler; it is actually a correctness issue. Those functions
> must not call into the tracer with AC=1

You are right!
I assumed they are defined in kasan.c and then call into a report
function that is compiled with -pg.
We can either disable -pg for report.c, or move these callbacks into
kasan.c which already has -pg disabled. I don't have strong preference
either way as long as the option works in all cases.

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Wed, Mar 06, 2019 at 02:13:47PM +0100, Peter Zijlstra wrote:
> +static __always_inline unsigned long smap_save(void)
> +{
> +	unsigned long flags = arch_local_save_flags();
> +	clac();
> +	return flags;
> +}
> +
> +static __always_inline void smap_restore(unsigned long flags)
> +{
> +	arch_local_irq_restore(flags);
> +}

ARGH; the bloody paravirt me harder nonsense makes that pvops calls.

And that (obviously) explodes.. Anybody got any clue why that Xen
trainwreck wants to paravirt: "PUSHF;POP" and "PUSH;POPF" !?

Also; I suppose I can ALTERNATIVE the whole thing, because Xen will not
be having SMAP in the first place I suppose.

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Linus Torvalds]

On Wed, Mar 6, 2019 at 9:14 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> And that (obviously) explodes.. Anybody got any clue why that Xen
> trainwreck wants to paravirt: "PUSHF;POP" and "PUSH;POPF" !?

Trying to regenerate the IF flag? I dunno.

But yeah, I think you could just force all of it into an explicit asm.
Or just use native_save_fl(). Except now that I look at it, for some
reason that is "extern inline" and we have an out-of-line option. I
can't for the life of me imagine why.

[ Looking at history. Oh, it's because of that same Xen issue and Xen
wanting to use it as a function pointer. Ugh. What a crock. ]

                  Linus

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Wed, Mar 06, 2019 at 06:14:51PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 06, 2019 at 02:13:47PM +0100, Peter Zijlstra wrote:
> > +static __always_inline unsigned long smap_save(void)
> > +{
> > +	unsigned long flags = arch_local_save_flags();
> > +	clac();
> > +	return flags;
> > +}
> > +
> > +static __always_inline void smap_restore(unsigned long flags)
> > +{
> > +	arch_local_irq_restore(flags);
> > +}
> 
> ARGH; the bloody paravirt me harder nonsense makes that pvops calls.
> 
> And that (obviously) explodes.. Anybody got any clue why that Xen
> trainwreck wants to paravirt: "PUSHF;POP" and "PUSH;POPF" !?
> 
> Also; I suppose I can ALTERNATIVE the whole thing, because Xen will not
> be having SMAP in the first place I suppose.

The below seems to 'work'.

--- a/arch/x86/include/asm/smap.h
+++ b/arch/x86/include/asm/smap.h
@@ -46,8 +46,6 @@
 
 #ifdef CONFIG_X86_SMAP
 
-#include <asm/irqflags.h>
-
 static __always_inline void clac(void)
 {
 	/* Note: a barrier is implicit in alternative() */
@@ -62,14 +60,19 @@ static __always_inline void stac(void)
 
 static __always_inline unsigned long smap_save(void)
 {
-	unsigned long flags = arch_local_save_flags();
-	clac();
+	unsigned long flags;
+
+	asm volatile (ALTERNATIVE("", "pushf; pop %0; " __stringify(__ASM_CLAC),
+				  X86_FEATURE_SMAP)
+		      : "=rm" (flags) : : "memory", "cc");
+
 	return flags;
 }
 
 static __always_inline void smap_restore(unsigned long flags)
 {
-	arch_local_irq_restore(flags);
+	asm volatile (ALTERNATIVE("", "push %0; popf", X86_FEATURE_SMAP)
+		      : : "g" (flags) : "memory", "cc");
 }
 
 /* These macros can be used in asm() statements */

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Linus Torvalds]

On Wed, Mar 6, 2019 at 9:37 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> The below seems to 'work'.

Yeah, and makes things cheaper for the non-SMAP case too. Looks sane.

One note:

 +       asm volatile (ALTERNATIVE("", "pushf; pop %0; "
__stringify(__ASM_CLAC),

Hmm. Every single use of __ASM_CLAC is together with "__stringity()".

Maybe we could just get rid of that oddity, and just make __ASM_CLAC
be a string to begin with.

At one point it was used bare in the __ASSEMBLY__ version, but that
does not appear to the case any more since commit 669f8a900198
("x86/smap: Use ALTERNATIVE macro") back in 2015.

                 Linus

Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Peter Zijlstra]

On Wed, Mar 06, 2019 at 09:59:11AM -0800, Linus Torvalds wrote:
> Maybe we could just get rid of that oddity, and just make __ASM_CLAC
> be a string to begin with.

Seems to work.. I'll add it to the pile.

---
 arch/x86/include/asm/smap.h |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

--- a/arch/x86/include/asm/smap.h
+++ b/arch/x86/include/asm/smap.h
@@ -13,13 +13,12 @@
 #ifndef _ASM_X86_SMAP_H
 #define _ASM_X86_SMAP_H
 
-#include <linux/stringify.h>
 #include <asm/nops.h>
 #include <asm/cpufeatures.h>
 
 /* "Raw" instruction opcodes */
-#define __ASM_CLAC	.byte 0x0f,0x01,0xca
-#define __ASM_STAC	.byte 0x0f,0x01,0xcb
+#define __ASM_CLAC	".byte 0x0f,0x01,0xca"
+#define __ASM_STAC	".byte 0x0f,0x01,0xcb"
 
 #ifdef __ASSEMBLY__
 
@@ -28,10 +27,10 @@
 #ifdef CONFIG_X86_SMAP
 
 #define ASM_CLAC \
-	ALTERNATIVE "", __stringify(__ASM_CLAC), X86_FEATURE_SMAP
+	ALTERNATIVE "", __ASM_CLAC, X86_FEATURE_SMAP
 
 #define ASM_STAC \
-	ALTERNATIVE "", __stringify(__ASM_STAC), X86_FEATURE_SMAP
+	ALTERNATIVE "", __ASM_STAC, X86_FEATURE_SMAP
 
 #else /* CONFIG_X86_SMAP */
 
@@ -49,20 +48,20 @@
 static __always_inline void clac(void)
 {
 	/* Note: a barrier is implicit in alternative() */
-	alternative("", __stringify(__ASM_CLAC), X86_FEATURE_SMAP);
+	alternative("", __ASM_CLAC, X86_FEATURE_SMAP);
 }
 
 static __always_inline void stac(void)
 {
 	/* Note: a barrier is implicit in alternative() */
-	alternative("", __stringify(__ASM_STAC), X86_FEATURE_SMAP);
+	alternative("", __ASM_STAC, X86_FEATURE_SMAP);
 }
 
 static __always_inline unsigned long smap_save(void)
 {
 	unsigned long flags;
 
-	asm volatile (ALTERNATIVE("", "pushf; pop %0; " __stringify(__ASM_CLAC),
+	asm volatile (ALTERNATIVE("", "pushf; pop %0; " __ASM_CLAC,
 				  X86_FEATURE_SMAP)
 		      : "=rm" (flags) : : "memory", "cc");
 
@@ -77,9 +76,9 @@ static __always_inline void smap_restore
 
 /* These macros can be used in asm() statements */
 #define ASM_CLAC \
-	ALTERNATIVE("", __stringify(__ASM_CLAC), X86_FEATURE_SMAP)
+	ALTERNATIVE("", __ASM_CLAC, X86_FEATURE_SMAP)
 #define ASM_STAC \
-	ALTERNATIVE("", __stringify(__ASM_STAC), X86_FEATURE_SMAP)
+	ALTERNATIVE("", __ASM_STAC, X86_FEATURE_SMAP)
 
 #else /* CONFIG_X86_SMAP */