[PATCH 0/7] x86/entry: noinstr fixes

[PATCH 0/7] x86/entry: noinstr fixes

[Peter Zijlstra]

Hi all,

These are the objtool/kcov patches that fix kcov-vs-noinstr and
a few patches that fix bad_iret noinstr issues.

I'm going to merge the objtool patches in objtool/urgent but make sure the
merge into objtool/core looks like the patches as posted here, since there's
the reloc conflicts.

The other patches I'm going to stick in x86/entry unless complaints.

[PATCH 1/7] x86/entry: Fix #UD vs WARN more

[Peter Zijlstra]

vmlinux.o: warning: objtool: exc_invalid_op()+0x47: call to probe_kernel_read() leaves .noinstr.text section

Since we use UD2 as a short-cut for 'CALL __WARN', treat it as such.
Have the bare exception handler do the report_bug() thing.

Fixes: 15a416e8aaa7 ("x86/entry: Treat BUG/WARN as NMI-like entries")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
---
 arch/x86/kernel/traps.c |   50 +++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -216,40 +216,34 @@ static inline void handle_invalid_op(str
 		      ILL_ILLOPN, error_get_trap_addr(regs));
 }
 
+static noinstr bool handle_bug(struct pt_regs *regs)
+{
+	bool handled = false;
+
+	/*
+	 * All lies, just get the WARN/BUG out.
+	 */
+	instrumentation_begin();
+	if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) {
+		regs->ip += LEN_UD2;
+		handled = true;
+	}
+	instrumentation_end();
+
+	return handled;
+}
+
 DEFINE_IDTENTRY_RAW(exc_invalid_op)
 {
 	bool rcu_exit;
 
 	/*
-	 * Handle BUG/WARN like NMIs instead of like normal idtentries:
-	 * if we bugged/warned in a bad RCU context, for example, the last
-	 * thing we want is to BUG/WARN again in the idtentry code, ad
-	 * infinitum.
+	 * We use UD2 as a short encoding for 'CALL __WARN', as such
+	 * handle it before exception entry to avoid recursive WARN
+	 * in case exception entry is the one triggering WARNs.
 	 */
-	if (!user_mode(regs) && is_valid_bugaddr(regs->ip)) {
-		enum bug_trap_type type;
-
-		nmi_enter();
-		instrumentation_begin();
-		trace_hardirqs_off_finish();
-		type = report_bug(regs->ip, regs);
-		if (regs->flags & X86_EFLAGS_IF)
-			trace_hardirqs_on_prepare();
-		instrumentation_end();
-		nmi_exit();
-
-		if (type == BUG_TRAP_TYPE_WARN) {
-			/* Skip the ud2. */
-			regs->ip += LEN_UD2;
-			return;
-		}
-
-		/*
-		 * Else, if this was a BUG and report_bug returns or if this
-		 * was just a normal #UD, we want to continue onward and
-		 * crash.
-		 */
-	}
+	if (!user_mode(regs) && handle_bug(regs))
+		return;
 
 	rcu_exit = idtentry_enter_cond_rcu(regs);
 	instrumentation_begin();

[PATCH 2/7] objtool: Dont consider vmlinux a C-file

[Peter Zijlstra]

Avoids issuing C-file warnings for vmlinux.

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

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2772,7 +2772,7 @@ int check(const char *_objname, bool orc
 
 	INIT_LIST_HEAD(&file.insn_list);
 	hash_init(file.insn_hash);
-	file.c_file = find_section_by_name(file.elf, ".comment");
+	file.c_file = !vmlinux && find_section_by_name(file.elf, ".comment");
 	file.ignore_unreachables = no_unreachable;
 	file.hints = false;
 

[PATCH 3/7] x86/entry: Fixup bad_iret vs noinstr

[Peter Zijlstra]

vmlinux.o: warning: objtool: fixup_bad_iret()+0x8e: call to memcpy() leaves .noinstr.text section

Worse, when KASAN there is no telling what memcpy() actually is. Force
the use of __memcpy() which is our assmebly implementation.

Reported-by: Marco Elver <elver@google.com>
Suggested-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/traps.c  |    6 +++---
 arch/x86/lib/memcpy_64.S |    4 ++++
 2 files changed, 7 insertions(+), 3 deletions(-)

--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -685,13 +685,13 @@ struct bad_iret_stack *fixup_bad_iret(st
 		(struct bad_iret_stack *)__this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
 
 	/* Copy the IRET target to the temporary storage. */
-	memcpy(&tmp.regs.ip, (void *)s->regs.sp, 5*8);
+	__memcpy(&tmp.regs.ip, (void *)s->regs.sp, 5*8);
 
 	/* Copy the remainder of the stack from the current stack. */
-	memcpy(&tmp, s, offsetof(struct bad_iret_stack, regs.ip));
+	__memcpy(&tmp, s, offsetof(struct bad_iret_stack, regs.ip));
 
 	/* Update the entry stack */
-	memcpy(new_stack, &tmp, sizeof(tmp));
+	__memcpy(new_stack, &tmp, sizeof(tmp));
 
 	BUG_ON(!user_mode(&new_stack->regs));
 	return new_stack;
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -8,6 +8,8 @@
 #include <asm/alternative-asm.h>
 #include <asm/export.h>
 
+.pushsection .noinstr.text, "ax"
+
 /*
  * We build a jump to memcpy_orig by default which gets NOPped out on
  * the majority of x86 CPUs which set REP_GOOD. In addition, CPUs which
@@ -184,6 +186,8 @@ SYM_FUNC_START_LOCAL(memcpy_orig)
 	retq
 SYM_FUNC_END(memcpy_orig)
 
+.popsection
+
 #ifndef CONFIG_UML
 
 MCSAFE_TEST_CTL

[PATCH 4/7] x86/entry: Increase entry_stack size to a full page

[Peter Zijlstra]

Marco crashed in bad_iret with a Clang11/KCSAN build due to
overflowing the stack. Now that we run C code on it, expand it to a
full page.

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Reported-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/processor.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -370,7 +370,7 @@ struct x86_hw_tss {
 #define IO_BITMAP_OFFSET_INVALID	(__KERNEL_TSS_LIMIT + 1)
 
 struct entry_stack {
-	unsigned long		words[64];
+	char	stack[PAGE_SIZE];
 };
 
 struct entry_stack_page {

[PATCH 5/7] objtool: Clean up elf_write() condition

[Peter Zijlstra]

With there being multiple ways to change the ELF data, let's more
concisely track modification.

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

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2740,7 +2740,7 @@ int check(const char *_objname, bool orc
 
 	objname = _objname;
 
-	file.elf = elf_open_read(objname, orc ? O_RDWR : O_RDONLY);
+	file.elf = elf_open_read(objname, O_RDWR);
 	if (!file.elf)
 		return 1;
 
@@ -2801,7 +2801,9 @@ int check(const char *_objname, bool orc
 		ret = create_orc_sections(&file);
 		if (ret < 0)
 			goto out;
+	}
 
+	if (file.elf->changed) {
 		ret = elf_write(file.elf);
 		if (ret < 0)
 			goto out;
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -747,6 +747,8 @@ struct section *elf_create_section(struc
 	elf_hash_add(elf->section_hash, &sec->hash, sec->idx);
 	elf_hash_add(elf->section_name_hash, &sec->name_hash, str_hash(sec->name));
 
+	elf->changed = true;
+
 	return sec;
 }
 
@@ -880,11 +882,14 @@ static int elf_rebuild_rela_reloc_sectio
 	return 0;
 }
 
-int elf_rebuild_reloc_section(struct section *sec)
+int elf_rebuild_reloc_section(struct elf *elf, struct section *sec)
 {
 	struct reloc *reloc;
 	int nr;
 
+	sec->changed = true;
+	elf->changed = true;
+
 	nr = 0;
 	list_for_each_entry(reloc, &sec->reloc_list, list)
 		nr++;
@@ -896,7 +901,7 @@ int elf_rebuild_reloc_section(struct sec
 	}
 }
 
-int elf_write(const struct elf *elf)
+int elf_write(struct elf *elf)
 {
 	struct section *sec;
 	Elf_Scn *s;
@@ -913,6 +918,8 @@ int elf_write(const struct elf *elf)
 				WARN_ELF("gelf_update_shdr");
 				return -1;
 			}
+
+			sec->changed = false;
 		}
 	}
 
@@ -925,6 +932,8 @@ int elf_write(const struct elf *elf)
 		return -1;
 	}
 
+	elf->changed = false;
+
 	return 0;
 }
 
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -79,6 +79,7 @@ struct elf {
 	Elf *elf;
 	GElf_Ehdr ehdr;
 	int fd;
+	bool changed;
 	char *name;
 	struct list_head sections;
 	DECLARE_HASHTABLE(symbol_hash, ELF_HASH_BITS);
@@ -121,7 +122,7 @@ struct elf *elf_open_read(const char *na
 struct section *elf_create_section(struct elf *elf, const char *name, size_t entsize, int nr);
 struct section *elf_create_reloc_section(struct elf *elf, struct section *base, int reltype);
 void elf_add_reloc(struct elf *elf, struct reloc *reloc);
-int elf_write(const struct elf *elf);
+int elf_write(struct elf *elf);
 void elf_close(struct elf *elf);
 
 struct section *find_section_by_name(const struct elf *elf, const char *name);
@@ -133,7 +134,7 @@ struct reloc *find_reloc_by_dest(const s
 struct reloc *find_reloc_by_dest_range(const struct elf *elf, struct section *sec,
 				     unsigned long offset, unsigned int len);
 struct symbol *find_func_containing(struct section *sec, unsigned long offset);
-int elf_rebuild_reloc_section(struct section *sec);
+int elf_rebuild_reloc_section(struct elf *elf, struct section *sec);
 
 #define for_each_sec(file, sec)						\
 	list_for_each_entry(sec, &file->elf->sections, list)
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -222,7 +222,7 @@ int create_orc_sections(struct objtool_f
 		}
 	}
 
-	if (elf_rebuild_reloc_section(ip_relocsec))
+	if (elf_rebuild_reloc_section(file->elf, ip_relocsec))
 		return -1;
 
 	return 0;

[PATCH 6/7] objtool: Provide elf_write_{insn,reloc}()

[Peter Zijlstra]

This provides infrastructure to rewrite instructions; this is
immediately useful for helping out with KCOV-vs-noinstr, but will
also come in handy for a bunch of variable sized jump-label patches
that are still on ice.

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

--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -114,6 +114,53 @@ static int symbol_to_offset(struct rb_no
 	return 0;
 }
 
+int elf_write_insn(struct elf *elf, struct section *sec,
+		   unsigned long offset, unsigned int len,
+		   const char *insn)
+{
+	Elf_Data *data = sec->data;
+
+	if (data->d_type != ELF_T_BYTE || data->d_off) {
+		WARN("write to unexpected data for section: %s", sec->name);
+		return -1;
+	}
+
+	memcpy(data->d_buf + offset, insn, len);
+	elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY);
+
+	elf->changed = true;
+
+	return 0;
+}
+
+int elf_write_reloc(struct elf *elf, struct reloc *reloc)
+{
+	struct section *sec = reloc->sec;
+
+	if (sec->sh.sh_type == SHT_REL) {
+		reloc->rel.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
+		reloc->rel.r_offset = reloc->offset;
+
+		if (!gelf_update_rel(sec->data, reloc->idx, &reloc->rel)) {
+			WARN_ELF("gelf_update_rel");
+			return -1;
+		}
+	} else {
+		reloc->rela.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
+		reloc->rela.r_addend = reloc->addend;
+		reloc->rela.r_offset = reloc->offset;
+
+		if (!gelf_update_rela(sec->data, reloc->idx, &reloc->rela)) {
+			WARN_ELF("gelf_update_rela");
+			return -1;
+		}
+	}
+
+	elf->changed = true;
+
+	return 0;
+}
+
 static int symbol_by_offset(const void *key, const struct rb_node *node)
 {
 	const struct symbol *s = rb_entry(node, struct symbol, node);
@@ -563,8 +610,9 @@ static int read_relocs(struct elf *elf)
 				break;
 			default: return -1;
 			}
-			reloc->sym = find_symbol_by_index(elf, symndx);
 			reloc->sec = sec;
+			reloc->idx = i;
+			reloc->sym = find_symbol_by_index(elf, symndx);
 			if (!reloc->sym) {
 				WARN("can't find reloc entry symbol %d for %s",
 				     symndx, sec->name);
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -67,9 +67,10 @@ struct reloc {
 	};
 	struct section *sec;
 	struct symbol *sym;
-	unsigned int type;
 	unsigned long offset;
+	unsigned int type;
 	int addend;
+	int idx;
 	bool jump_table_start;
 };
 
@@ -122,6 +123,10 @@ struct elf *elf_open_read(const char *na
 struct section *elf_create_section(struct elf *elf, const char *name, size_t entsize, int nr);
 struct section *elf_create_reloc_section(struct elf *elf, struct section *base, int reltype);
 void elf_add_reloc(struct elf *elf, struct reloc *reloc);
+int elf_write_insn(struct elf *elf, struct section *sec,
+		   unsigned long offset, unsigned int len,
+		   const char *insn);
+int elf_write_reloc(struct elf *elf, struct reloc *reloc);
 int elf_write(struct elf *elf);
 void elf_close(struct elf *elf);
 

[PATCH 7/7] objtool: Fix noinstr vs KCOV

[Peter Zijlstra]

Since many compilers cannot disable KCOV with a function attribute,
help it to NOP out any __sanitizer_cov_*() calls injected in noinstr
code.

This turns:

12:   e8 00 00 00 00          callq  17 <lockdep_hardirqs_on+0x17>
		13: R_X86_64_PLT32      __sanitizer_cov_trace_pc-0x4

into:

12:   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
		13: R_X86_64_NONE      __sanitizer_cov_trace_pc-0x4

Just like recordmcount does.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Dmitry Vyukov <dvyukov@google.com>
---
 arch/x86/Kconfig                          |    2 +-
 tools/objtool/arch.h                      |    2 ++
 tools/objtool/arch/x86/decode.c           |   18 ++++++++++++++++++
 tools/objtool/arch/x86/include/arch_elf.h |    6 ++++++
 tools/objtool/check.c                     |   19 +++++++++++++++++++
 5 files changed, 46 insertions(+), 1 deletion(-)

--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -67,7 +67,7 @@ config X86
 	select ARCH_HAS_FILTER_PGPROT
 	select ARCH_HAS_FORTIFY_SOURCE
 	select ARCH_HAS_GCOV_PROFILE_ALL
-	select ARCH_HAS_KCOV			if X86_64
+	select ARCH_HAS_KCOV			if X86_64 && STACK_VALIDATION
 	select ARCH_HAS_MEM_ENCRYPT
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
 	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -84,4 +84,6 @@ unsigned long arch_jump_destination(stru
 
 unsigned long arch_dest_reloc_offset(int addend);
 
+const char *arch_nop_insn(int len);
+
 #endif /* _ARCH_H */
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -565,3 +565,21 @@ void arch_initial_func_cfi_state(struct
 	state->regs[16].base = CFI_CFA;
 	state->regs[16].offset = -8;
 }
+
+const char *arch_nop_insn(int len)
+{
+	static const char nops[5][5] = {
+		/* 1 */ { 0x90 },
+		/* 2 */ { 0x66, 0x90 },
+		/* 3 */ { 0x0f, 0x1f, 0x00 },
+		/* 4 */ { 0x0f, 0x1f, 0x40, 0x00 },
+		/* 5 */ { 0x0f, 0x1f, 0x44, 0x00, 0x00 },
+	};
+
+	if (len < 1 || len > 5) {
+		WARN("invalid NOP size: %d\n", len);
+		return NULL;
+	}
+
+	return nops[len-1];
+}
--- /dev/null
+++ b/tools/objtool/arch/x86/include/arch_elf.h
@@ -0,0 +1,6 @@
+#ifndef _OBJTOOL_ARCH_ELF
+#define _OBJTOOL_ARCH_ELF
+
+#define R_NONE R_X86_64_NONE
+
+#endif /* _OBJTOOL_ARCH_ELF */
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -12,6 +12,7 @@
 #include "check.h"
 #include "special.h"
 #include "warn.h"
+#include "arch_elf.h"
 
 #include <linux/hashtable.h>
 #include <linux/kernel.h>
@@ -766,6 +767,24 @@ static int add_call_destinations(struct
 			insn->call_dest = reloc->sym;
 
 		/*
+		 * Many compilers cannot disable KCOV with a function attribute
+		 * so they need a little help, NOP out any KCOV calls from noinstr
+		 * text.
+		 */
+		if (insn->sec->noinstr &&
+		    !strncmp(insn->call_dest->name, "__sanitizer_cov_", 16)) {
+			if (reloc) {
+				reloc->type = R_NONE;
+				elf_write_reloc(file->elf, reloc);
+			}
+
+			elf_write_insn(file->elf, insn->sec,
+				       insn->offset, insn->len,
+				       arch_nop_insn(insn->len));
+			insn->type = INSN_NOP;
+		}
+
+		/*
 		 * Whatever stack impact regular CALLs have, should be undone
 		 * by the RETURN of the called function.
 		 *

Re: [PATCH 1/7] x86/entry: Fix #UD vs WARN more

[Andy Lutomirski]

> On Jun 18, 2020, at 7:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> vmlinux.o: warning: objtool: exc_invalid_op()+0x47: call to probe_kernel_read() leaves .noinstr.text section
> 
> Since we use UD2 as a short-cut for 'CALL __WARN', treat it as such.
> Have the bare exception handler do the report_bug() thing.

I think you should consider inlining or noinstr-ifying report_bug() too if you want to make this more bulletproof. I admit the scenario where someone instruments it and it goes wrong is farfetched.

> 
> Fixes: 15a416e8aaa7 ("x86/entry: Treat BUG/WARN as NMI-like entries")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> ---
> arch/x86/kernel/traps.c |   50 +++++++++++++++++++++---------------------------
> 1 file changed, 22 insertions(+), 28 deletions(-)
> 
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -216,40 +216,34 @@ static inline void handle_invalid_op(str
>              ILL_ILLOPN, error_get_trap_addr(regs));
> }
> 
> +static noinstr bool handle_bug(struct pt_regs *regs)
> +{
> +    bool handled = false;
> +
> +    /*
> +     * All lies, just get the WARN/BUG out.
> +     */
> +    instrumentation_begin();
> +    if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) {
> +        regs->ip += LEN_UD2;
> +        handled = true;
> +    }
> +    instrumentation_end();
> +
> +    return handled;
> +}
> +
> DEFINE_IDTENTRY_RAW(exc_invalid_op)
> {
>    bool rcu_exit;
> 
>    /*
> -     * Handle BUG/WARN like NMIs instead of like normal idtentries:
> -     * if we bugged/warned in a bad RCU context, for example, the last
> -     * thing we want is to BUG/WARN again in the idtentry code, ad
> -     * infinitum.
> +     * We use UD2 as a short encoding for 'CALL __WARN', as such
> +     * handle it before exception entry to avoid recursive WARN
> +     * in case exception entry is the one triggering WARNs.
>     */
> -    if (!user_mode(regs) && is_valid_bugaddr(regs->ip)) {
> -        enum bug_trap_type type;
> -
> -        nmi_enter();
> -        instrumentation_begin();
> -        trace_hardirqs_off_finish();
> -        type = report_bug(regs->ip, regs);
> -        if (regs->flags & X86_EFLAGS_IF)
> -            trace_hardirqs_on_prepare();
> -        instrumentation_end();
> -        nmi_exit();
> -
> -        if (type == BUG_TRAP_TYPE_WARN) {
> -            /* Skip the ud2. */
> -            regs->ip += LEN_UD2;
> -            return;
> -        }
> -
> -        /*
> -         * Else, if this was a BUG and report_bug returns or if this
> -         * was just a normal #UD, we want to continue onward and
> -         * crash.
> -         */
> -    }
> +    if (!user_mode(regs) && handle_bug(regs))
> +        return;
> 
>    rcu_exit = idtentry_enter_cond_rcu(regs);
>    instrumentation_begin();
> 
> 

Re: [PATCH 4/7] x86/entry: Increase entry_stack size to a full page

[Marco Elver]

On Thu, 18 Jun 2020 at 16:50, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Marco crashed in bad_iret with a Clang11/KCSAN build due to
> overflowing the stack. Now that we run C code on it, expand it to a
> full page.
>
> Suggested-by: Andy Lutomirski <luto@amacapital.net>
> Reported-by: Marco Elver <elver@google.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

This fixes the unpredictable crashes.

Tested-by: Marco Elver <elver@google.com>

Thanks!


> ---
>  arch/x86/include/asm/processor.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -370,7 +370,7 @@ struct x86_hw_tss {
>  #define IO_BITMAP_OFFSET_INVALID       (__KERNEL_TSS_LIMIT + 1)
>
>  struct entry_stack {
> -       unsigned long           words[64];
> +       char    stack[PAGE_SIZE];
>  };
>
>  struct entry_stack_page {
>
>

Re: [PATCH 3/7] x86/entry: Fixup bad_iret vs noinstr

[Marco Elver]

On Thu, 18 Jun 2020 at 16:50, Peter Zijlstra <peterz@infradead.org> wrote:
>
> vmlinux.o: warning: objtool: fixup_bad_iret()+0x8e: call to memcpy() leaves .noinstr.text section
>
> Worse, when KASAN there is no telling what memcpy() actually is. Force
> the use of __memcpy() which is our assmebly implementation.
>
> Reported-by: Marco Elver <elver@google.com>
> Suggested-by: Marco Elver <elver@google.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

KASAN no longer crashes, although the stack size increase appears to
be sufficient for the particular case I ran into.

Tested-by: Marco Elver <elver@google.com>

Thanks!

> ---
>  arch/x86/kernel/traps.c  |    6 +++---
>  arch/x86/lib/memcpy_64.S |    4 ++++
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -685,13 +685,13 @@ struct bad_iret_stack *fixup_bad_iret(st
>                 (struct bad_iret_stack *)__this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
>
>         /* Copy the IRET target to the temporary storage. */
> -       memcpy(&tmp.regs.ip, (void *)s->regs.sp, 5*8);
> +       __memcpy(&tmp.regs.ip, (void *)s->regs.sp, 5*8);
>
>         /* Copy the remainder of the stack from the current stack. */
> -       memcpy(&tmp, s, offsetof(struct bad_iret_stack, regs.ip));
> +       __memcpy(&tmp, s, offsetof(struct bad_iret_stack, regs.ip));
>
>         /* Update the entry stack */
> -       memcpy(new_stack, &tmp, sizeof(tmp));
> +       __memcpy(new_stack, &tmp, sizeof(tmp));
>
>         BUG_ON(!user_mode(&new_stack->regs));
>         return new_stack;
> --- a/arch/x86/lib/memcpy_64.S
> +++ b/arch/x86/lib/memcpy_64.S
> @@ -8,6 +8,8 @@
>  #include <asm/alternative-asm.h>
>  #include <asm/export.h>
>
> +.pushsection .noinstr.text, "ax"
> +
>  /*
>   * We build a jump to memcpy_orig by default which gets NOPped out on
>   * the majority of x86 CPUs which set REP_GOOD. In addition, CPUs which
> @@ -184,6 +186,8 @@ SYM_FUNC_START_LOCAL(memcpy_orig)
>         retq
>  SYM_FUNC_END(memcpy_orig)
>
> +.popsection
> +
>  #ifndef CONFIG_UML
>
>  MCSAFE_TEST_CTL
>
>

Re: [PATCH 1/7] x86/entry: Fix #UD vs WARN more

[Peter Zijlstra]

On Thu, Jun 18, 2020 at 07:57:35AM -0700, Andy Lutomirski wrote:
> 
> 
> > On Jun 18, 2020, at 7:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > vmlinux.o: warning: objtool: exc_invalid_op()+0x47: call to probe_kernel_read() leaves .noinstr.text section
> > 
> > Since we use UD2 as a short-cut for 'CALL __WARN', treat it as such.
> > Have the bare exception handler do the report_bug() thing.
> 
> I think you should consider inlining or noinstr-ifying report_bug()
> too if you want to make this more bulletproof. I admit the scenario
> where someone instruments it and it goes wrong is farfetched.

How far down that rabbit hole do we go? Because then we need to noinstr
printk, the console drivers, those will very quickly pull in lovely bits
like PCI, USB, DRM :/

At some point we have to just give up.

Re: [PATCH 1/7] x86/entry: Fix #UD vs WARN more

[Andy Lutomirski]

On Thu, Jun 18, 2020 at 8:50 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jun 18, 2020 at 07:57:35AM -0700, Andy Lutomirski wrote:
> >
> >
> > > On Jun 18, 2020, at 7:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > vmlinux.o: warning: objtool: exc_invalid_op()+0x47: call to probe_kernel_read() leaves .noinstr.text section
> > >
> > > Since we use UD2 as a short-cut for 'CALL __WARN', treat it as such.
> > > Have the bare exception handler do the report_bug() thing.
> >
> > I think you should consider inlining or noinstr-ifying report_bug()
> > too if you want to make this more bulletproof. I admit the scenario
> > where someone instruments it and it goes wrong is farfetched.
>
> How far down that rabbit hole do we go? Because then we need to noinstr
> printk, the console drivers, those will very quickly pull in lovely bits
> like PCI, USB, DRM :/
>
> At some point we have to just give up.

I wasn't imagining going far down the rabbit hole at all -- I think
that, at most, we should cover the path for when the fault wasn't a
BUG/WARN in the first place.  I admit that, for #UD in particular,
this isn't a big deal, but if it were a different vector, this could
matter.

I suppose report_bug() could be split into lookup_bug() and
handle_bug(), and just lookup_bug() could be noinstr.

--Andy

Re: [PATCH 1/7] x86/entry: Fix #UD vs WARN more

[Peter Zijlstra]

On Thu, Jun 18, 2020 at 11:36:53AM -0700, Andy Lutomirski wrote:

> I wasn't imagining going far down the rabbit hole at all -- I think
> that, at most, we should cover the path for when the fault wasn't a
> BUG/WARN in the first place.  I admit that, for #UD in particular,
> this isn't a big deal, but if it were a different vector, this could
> matter.

Right, so there's 3 cases for ud2:

 - WARN;  ud2,  bug_entry, recovers
 - BUG;   ud2,  bug_entry, dies
 - UBSAN; ud2, !bug_entry, dies

Nothing else should be generating ud2 instructions, any other #UD goes
into handle_invalid_op() -> do_error_trap() -> ... -> die().

[ while there, we should probably restructure do_trap() to have
  cond_local_irq_enable() _after_ do_trap_no_signal(). ]

We could probably change is_valid_bugaddr() to not use
probe_kernel_address(), because if it couldn't read the instruction,
we'd not be getting #UD in the first place.

If we've gotten rid of probe_kernel_address() we can noinstr/inline that
and then we can only call into report_bug() IFF ud2.

Does that make things 'better' ? This can only go realy bad if there's a
1 byte instruction that triggers #UD, but I think that was ruled out.


---
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c26751e303f1..275a621f1aff 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -91,10 +91,7 @@ int is_valid_bugaddr(unsigned long addr)
 	if (addr < TASK_SIZE_MAX)
 		return 0;
 
-	if (probe_kernel_address((unsigned short *)addr, ud))
-		return 0;
-
-	return ud == INSN_UD0 || ud == INSN_UD2;
+	return *(unsigned short *)addr == INSN_UD2;
 }
 
 static nokprobe_inline int
@@ -220,15 +217,17 @@ static noinstr bool handle_bug(struct pt_regs *regs)
 {
 	bool handled = false;
 
-	/*
-	 * All lies, just get the WARN/BUG out.
-	 */
-	instrumentation_begin();
-	if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) {
-		regs->ip += LEN_UD2;
-		handled = true;
+	if (is_valid_bugaddr(regs->ip)) {
+		/*
+		 * All lies, just get the WARN/BUG out.
+		 */
+		instrumentation_begin();
+		if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) {
+			regs->ip += LEN_UD2;
+			handled = true;
+		}
+		instrumentation_end();
 	}
-	instrumentation_end();
 
 	return handled;
 }

Re: [PATCH 1/7] x86/entry: Fix #UD vs WARN more

[Andy Lutomirski]

> On Jun 18, 2020, at 12:02 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Jun 18, 2020 at 11:36:53AM -0700, Andy Lutomirski wrote:
> 
>> I wasn't imagining going far down the rabbit hole at all -- I think
>> that, at most, we should cover the path for when the fault wasn't a
>> BUG/WARN in the first place.  I admit that, for #UD in particular,
>> this isn't a big deal, but if it were a different vector, this could
>> matter.
> 
> Right, so there's 3 cases for ud2:
> 
> - WARN;  ud2,  bug_entry, recovers
> - BUG;   ud2,  bug_entry, dies
> - UBSAN; ud2, !bug_entry, dies

4. The #UD matches an extable entry. I don’t know whether this ever happens for real.

The failure is still a bit farfetched: we’d need an extable to hit in an inconsistent state where we blow up due to a lack of entry handling.

> 
> Nothing else should be generating ud2 instructions, any other #UD goes
> into handle_invalid_op() -> do_error_trap() -> ... -> die().
> 
> [ while there, we should probably restructure do_trap() to have
>  cond_local_irq_enable() _after_ do_trap_no_signal(). ]
> 
> We could probably change is_valid_bugaddr() to not use
> probe_kernel_address(), because if it couldn't read the instruction,
> we'd not be getting #UD in the first place.
> 
> If we've gotten rid of probe_kernel_address() we can noinstr/inline that
> and then we can only call into report_bug() IFF ud2.
> 
> Does that make things 'better' ? This can only go realy bad if there's a
> 1 byte instruction that triggers #UD, but I think that was ruled out.
> 
> 
> ---
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index c26751e303f1..275a621f1aff 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -91,10 +91,7 @@ int is_valid_bugaddr(unsigned long addr)
>    if (addr < TASK_SIZE_MAX)
>        return 0;
> 
> -    if (probe_kernel_address((unsigned short *)addr, ud))
> -        return 0;
> -
> -    return ud == INSN_UD0 || ud == INSN_UD2;
> +    return *(unsigned short *)addr == INSN_UD2;
> }

I’m okay with this, at least until we get PKRS or kernel XO memory. But probe_kernel_addr() would be wrong then, too.  We need probe_kernel_text().

But I think you might need some IRQ fiddling. With your patch, a WARN with IRQs on will execute the printk code with IRQs off without lockstep handling, and an appropriately configured debugging kernel may get a recursive splat.  Or if irq tracing somehow notices that IRQs got turned off, the warning recovery might return back to an IF=1 context with IRQs traced as off.

So maybe also do an untraced cond_local_irq_enable()?  After all, if we’re trying to report a bug from IRQs on, it should be okay to have IRQs on while reporting it. It might even work better than having IRQs off.

> 
> static nokprobe_inline int
> @@ -220,15 +217,17 @@ static noinstr bool handle_bug(struct pt_regs *regs)
> {
>    bool handled = false;
> 
> -    /*
> -     * All lies, just get the WARN/BUG out.
> -     */
> -    instrumentation_begin();
> -    if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) {
> -        regs->ip += LEN_UD2;
> -        handled = true;
> +    if (is_valid_bugaddr(regs->ip)) {
> +        /*
> +         * All lies, just get the WARN/BUG out.
> +         */
> +        instrumentation_begin();
> +        if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) {
> +            regs->ip += LEN_UD2;
> +            handled = true;
> +        }
> +        instrumentation_end();
>    }
> -    instrumentation_end();
> 
>    return handled;
> }

Re: [PATCH 1/7] x86/entry: Fix #UD vs WARN more

[Peter Zijlstra]

On Thu, Jun 18, 2020 at 12:29:50PM -0700, Andy Lutomirski wrote:
> 
> > On Jun 18, 2020, at 12:02 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > On Thu, Jun 18, 2020 at 11:36:53AM -0700, Andy Lutomirski wrote:
> > 
> >> I wasn't imagining going far down the rabbit hole at all -- I think
> >> that, at most, we should cover the path for when the fault wasn't a
> >> BUG/WARN in the first place.  I admit that, for #UD in particular,
> >> this isn't a big deal, but if it were a different vector, this could
> >> matter.
> > 
> > Right, so there's 3 cases for ud2:
> > 
> > - WARN;  ud2,  bug_entry, recovers
> > - BUG;   ud2,  bug_entry, dies
> > - UBSAN; ud2, !bug_entry, dies
> 
> 4. The #UD matches an extable entry. I don’t know whether this ever happens for real.

#UD yes, ud2 instruction, not so much.

> The failure is still a bit farfetched: we’d need an extable to hit in
> an inconsistent state where we blow up due to a lack of entry
> handling.

Right, by noinstr checking the instruction is actually ud2 I think we
mostly good. There really aren't that many places that emit ud2.

> But I think you might need some IRQ fiddling. With your patch, a WARN
> with IRQs on will execute the printk code with IRQs off without
> lockstep handling, and an appropriately configured debugging kernel
> may get a recursive splat.  Or if irq tracing somehow notices that
> IRQs got turned off, the warning recovery might return back to an IF=1
> context with IRQs traced as off.
> 
> So maybe also do an untraced cond_local_irq_enable()?  After all, if
> we’re trying to report a bug from IRQs on, it should be okay to have
> IRQs on while reporting it. It might even work better than having IRQs
> off.

Yes, very good point. Now I want to go look at the old code... I'll frob
something tomorrow, brain is pretty fried by now.

Re: [PATCH 4/7] x86/entry: Increase entry_stack size to a full page

[Lai Jiangshan]

On Fri, Jun 19, 2020 at 4:37 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Marco crashed in bad_iret with a Clang11/KCSAN build due to
> overflowing the stack. Now that we run C code on it, expand it to a
> full page.

Some of my experimental code also once got crashed due to overflowing
the stack. I'm glad to have the stack expanded, thanks.

Reviewed-by: Lai Jiangshan <jiangshanlai@gmail.com>

>
> Suggested-by: Andy Lutomirski <luto@amacapital.net>
> Reported-by: Marco Elver <elver@google.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/processor.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -370,7 +370,7 @@ struct x86_hw_tss {
>  #define IO_BITMAP_OFFSET_INVALID       (__KERNEL_TSS_LIMIT + 1)
>
>  struct entry_stack {
> -       unsigned long           words[64];
> +       char    stack[PAGE_SIZE];
>  };
>
>  struct entry_stack_page {
>
>

Re: [PATCH 1/7] x86/entry: Fix #UD vs WARN more

[Peter Zijlstra]

On Thu, Jun 18, 2020 at 11:18:23PM +0200, Peter Zijlstra wrote:

> > So maybe also do an untraced cond_local_irq_enable()?  After all, if
> > we’re trying to report a bug from IRQs on, it should be okay to have
> > IRQs on while reporting it. It might even work better than having IRQs
> > off.
> 
> Yes, very good point. Now I want to go look at the old code... I'll frob
> something tomorrow, brain is pretty fried by now.

How's this then?

---
Subject: x86/entry: Fix #UD vs WARN more
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Jun 16 13:28:36 CEST 2020

vmlinux.o: warning: objtool: exc_invalid_op()+0x47: call to probe_kernel_read() leaves .noinstr.text section

Since we use UD2 as a short-cut for 'CALL __WARN', treat it as such.
Have the bare exception handler do the report_bug() thing.

Fixes: 15a416e8aaa7 ("x86/entry: Treat BUG/WARN as NMI-like entries")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/traps.c |   72 +++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 34 deletions(-)

--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -86,15 +86,14 @@ static inline void cond_local_irq_disabl
 
 int is_valid_bugaddr(unsigned long addr)
 {
-	unsigned short ud;
-
 	if (addr < TASK_SIZE_MAX)
 		return 0;
 
-	if (probe_kernel_address((unsigned short *)addr, ud))
-		return 0;
-
-	return ud == INSN_UD0 || ud == INSN_UD2;
+	/*
+	 * We got #UD, if the text isn't readable we'd have gotten
+	 * a different exception.
+	 */
+	return *(unsigned short)addr == INSN_UD2;
 }
 
 static nokprobe_inline int
@@ -216,40 +215,45 @@ static inline void handle_invalid_op(str
 		      ILL_ILLOPN, error_get_trap_addr(regs));
 }
 
-DEFINE_IDTENTRY_RAW(exc_invalid_op)
+static noinstr bool handle_bug(struct pt_regs *regs)
 {
-	bool rcu_exit;
+	bool handled = false;
+
+	if (!is_valid_bugaddr(regs->ip))
+		return handled;
 
 	/*
-	 * Handle BUG/WARN like NMIs instead of like normal idtentries:
-	 * if we bugged/warned in a bad RCU context, for example, the last
-	 * thing we want is to BUG/WARN again in the idtentry code, ad
-	 * infinitum.
+	 * All lies, just get the WARN/BUG out.
+	 */
+	instrumentation_begin();
+	/*
+	 * Since we're emulating a CALL with exceptions, restore the interrupt
+	 * state to what it was at the exception site.
 	 */
-	if (!user_mode(regs) && is_valid_bugaddr(regs->ip)) {
-		enum bug_trap_type type;
+	if (regs->flags & X86_EFLAGS_IF)
+		raw_local_irq_enable();
+	if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) {
+		regs->ip += LEN_UD2;
+		handled = true;
+	}
+	if (regs->flags & X86_EFLAGS_IF)
+		raw_local_irq_disable();
+	instrumentation_end();
 
-		nmi_enter();
-		instrumentation_begin();
-		trace_hardirqs_off_finish();
-		type = report_bug(regs->ip, regs);
-		if (regs->flags & X86_EFLAGS_IF)
-			trace_hardirqs_on_prepare();
-		instrumentation_end();
-		nmi_exit();
-
-		if (type == BUG_TRAP_TYPE_WARN) {
-			/* Skip the ud2. */
-			regs->ip += LEN_UD2;
-			return;
-		}
+	return handled;
+}
 
-		/*
-		 * Else, if this was a BUG and report_bug returns or if this
-		 * was just a normal #UD, we want to continue onward and
-		 * crash.
-		 */
-	}
+DEFINE_IDTENTRY_RAW(exc_invalid_op)
+{
+	bool rcu_exit;
+
+	/*
+	 * We use UD2 as a short encoding for 'CALL __WARN', as such
+	 * handle it before exception entry to avoid recursive WARN
+	 * in case exception entry is the one triggering WARNs.
+	 */
+	if (!user_mode(regs) && handle_bug(regs))
+		return;
 
 	rcu_exit = idtentry_enter_cond_rcu(regs);
 	instrumentation_begin();

Re: [PATCH 1/7] x86/entry: Fix #UD vs WARN more

[Andy Lutomirski]

On Mon, Jun 22, 2020 at 4:47 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jun 18, 2020 at 11:18:23PM +0200, Peter Zijlstra wrote:
>
> > > So maybe also do an untraced cond_local_irq_enable()?  After all, if
> > > we’re trying to report a bug from IRQs on, it should be okay to have
> > > IRQs on while reporting it. It might even work better than having IRQs
> > > off.
> >
> > Yes, very good point. Now I want to go look at the old code... I'll frob
> > something tomorrow, brain is pretty fried by now.
>
> How's this then?
>
> ---
> Subject: x86/entry: Fix #UD vs WARN more
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue Jun 16 13:28:36 CEST 2020
>
> vmlinux.o: warning: objtool: exc_invalid_op()+0x47: call to probe_kernel_read() leaves .noinstr.text section
>
> Since we use UD2 as a short-cut for 'CALL __WARN', treat it as such.
> Have the bare exception handler do the report_bug() thing.
>
> Fixes: 15a416e8aaa7 ("x86/entry: Treat BUG/WARN as NMI-like entries")

Acked-by: Andy Lutomirski <luto@kernel.org>

[tip: x86/entry] x86/entry: Increase entry_stack size to a full page

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/entry branch of tip:

Commit-ID:     c7aadc09321d8f9a1d3bd1e6d8a47222ecddf6c5
Gitweb:        https://git.kernel.org/tip/c7aadc09321d8f9a1d3bd1e6d8a47222ecddf6c5
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 17 Jun 2020 18:25:57 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 25 Jun 2020 13:45:40 +02:00

x86/entry: Increase entry_stack size to a full page

Marco crashed in bad_iret with a Clang11/KCSAN build due to
overflowing the stack. Now that we run C code on it, expand it to a
full page.

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Reported-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Lai Jiangshan <jiangshanlai@gmail.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200618144801.819246178@infradead.org
---
 arch/x86/include/asm/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 42cd333..03b7c4c 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -370,7 +370,7 @@ struct x86_hw_tss {
 #define IO_BITMAP_OFFSET_INVALID	(__KERNEL_TSS_LIMIT + 1)
 
 struct entry_stack {
-	unsigned long		words[64];
+	char	stack[PAGE_SIZE];
 };
 
 struct entry_stack_page {

[tip: x86/entry] x86/entry: Fix #UD vs WARN more

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/entry branch of tip:

Commit-ID:     145a773aef83181d47ebab21bb33c89233aadb1e
Gitweb:        https://git.kernel.org/tip/145a773aef83181d47ebab21bb33c89233aadb1e
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 16 Jun 2020 13:28:36 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 25 Jun 2020 13:45:40 +02:00

x86/entry: Fix #UD vs WARN more

vmlinux.o: warning: objtool: exc_invalid_op()+0x47: call to probe_kernel_read() leaves .noinstr.text section

Since we use UD2 as a short-cut for 'CALL __WARN', treat it as such.
Have the bare exception handler do the report_bug() thing.

Fixes: 15a416e8aaa7 ("x86/entry: Treat BUG/WARN as NMI-like entries")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Andy Lutomirski <luto@kernel.org>
Link: https://lkml.kernel.org/r/20200622114713.GE577403@hirez.programming.kicks-ass.net
---
 arch/x86/kernel/traps.c | 72 +++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 34 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index a7d1570..1d9ea21 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -84,17 +84,16 @@ static inline void cond_local_irq_disable(struct pt_regs *regs)
 		local_irq_disable();
 }
 
-int is_valid_bugaddr(unsigned long addr)
+__always_inline int is_valid_bugaddr(unsigned long addr)
 {
-	unsigned short ud;
-
 	if (addr < TASK_SIZE_MAX)
 		return 0;
 
-	if (probe_kernel_address((unsigned short *)addr, ud))
-		return 0;
-
-	return ud == INSN_UD0 || ud == INSN_UD2;
+	/*
+	 * We got #UD, if the text isn't readable we'd have gotten
+	 * a different exception.
+	 */
+	return *(unsigned short *)addr == INSN_UD2;
 }
 
 static nokprobe_inline int
@@ -216,40 +215,45 @@ static inline void handle_invalid_op(struct pt_regs *regs)
 		      ILL_ILLOPN, error_get_trap_addr(regs));
 }
 
-DEFINE_IDTENTRY_RAW(exc_invalid_op)
+static noinstr bool handle_bug(struct pt_regs *regs)
 {
-	bool rcu_exit;
+	bool handled = false;
+
+	if (!is_valid_bugaddr(regs->ip))
+		return handled;
 
 	/*
-	 * Handle BUG/WARN like NMIs instead of like normal idtentries:
-	 * if we bugged/warned in a bad RCU context, for example, the last
-	 * thing we want is to BUG/WARN again in the idtentry code, ad
-	 * infinitum.
+	 * All lies, just get the WARN/BUG out.
 	 */
-	if (!user_mode(regs) && is_valid_bugaddr(regs->ip)) {
-		enum bug_trap_type type;
+	instrumentation_begin();
+	/*
+	 * Since we're emulating a CALL with exceptions, restore the interrupt
+	 * state to what it was at the exception site.
+	 */
+	if (regs->flags & X86_EFLAGS_IF)
+		raw_local_irq_enable();
+	if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) {
+		regs->ip += LEN_UD2;
+		handled = true;
+	}
+	if (regs->flags & X86_EFLAGS_IF)
+		raw_local_irq_disable();
+	instrumentation_end();
 
-		nmi_enter();
-		instrumentation_begin();
-		trace_hardirqs_off_finish();
-		type = report_bug(regs->ip, regs);
-		if (regs->flags & X86_EFLAGS_IF)
-			trace_hardirqs_on_prepare();
-		instrumentation_end();
-		nmi_exit();
+	return handled;
+}
 
-		if (type == BUG_TRAP_TYPE_WARN) {
-			/* Skip the ud2. */
-			regs->ip += LEN_UD2;
-			return;
-		}
+DEFINE_IDTENTRY_RAW(exc_invalid_op)
+{
+	bool rcu_exit;
 
-		/*
-		 * Else, if this was a BUG and report_bug returns or if this
-		 * was just a normal #UD, we want to continue onward and
-		 * crash.
-		 */
-	}
+	/*
+	 * We use UD2 as a short encoding for 'CALL __WARN', as such
+	 * handle it before exception entry to avoid recursive WARN
+	 * in case exception entry is the one triggering WARNs.
+	 */
+	if (!user_mode(regs) && handle_bug(regs))
+		return;
 
 	rcu_exit = idtentry_enter_cond_rcu(regs);
 	instrumentation_begin();

[tip: x86/entry] x86/entry: Fixup bad_iret vs noinstr

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/entry branch of tip:

Commit-ID:     e3a9e681adb779b39565a28b3252c3be1033f994
Gitweb:        https://git.kernel.org/tip/e3a9e681adb779b39565a28b3252c3be1033f994
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 17 Jun 2020 18:21:16 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 25 Jun 2020 13:45:39 +02:00

x86/entry: Fixup bad_iret vs noinstr

vmlinux.o: warning: objtool: fixup_bad_iret()+0x8e: call to memcpy() leaves .noinstr.text section

Worse, when KASAN there is no telling what memcpy() actually is. Force
the use of __memcpy() which is our assmebly implementation.

Reported-by: Marco Elver <elver@google.com>
Suggested-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200618144801.760070502@infradead.org
---
 arch/x86/kernel/traps.c  | 6 +++---
 arch/x86/lib/memcpy_64.S | 4 ++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index af75109..a7d1570 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -690,13 +690,13 @@ struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
 		(struct bad_iret_stack *)__this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
 
 	/* Copy the IRET target to the temporary storage. */
-	memcpy(&tmp.regs.ip, (void *)s->regs.sp, 5*8);
+	__memcpy(&tmp.regs.ip, (void *)s->regs.sp, 5*8);
 
 	/* Copy the remainder of the stack from the current stack. */
-	memcpy(&tmp, s, offsetof(struct bad_iret_stack, regs.ip));
+	__memcpy(&tmp, s, offsetof(struct bad_iret_stack, regs.ip));
 
 	/* Update the entry stack */
-	memcpy(new_stack, &tmp, sizeof(tmp));
+	__memcpy(new_stack, &tmp, sizeof(tmp));
 
 	BUG_ON(!user_mode(&new_stack->regs));
 	return new_stack;
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index 56b243b..bbcc05b 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -8,6 +8,8 @@
 #include <asm/alternative-asm.h>
 #include <asm/export.h>
 
+.pushsection .noinstr.text, "ax"
+
 /*
  * We build a jump to memcpy_orig by default which gets NOPped out on
  * the majority of x86 CPUs which set REP_GOOD. In addition, CPUs which
@@ -184,6 +186,8 @@ SYM_FUNC_START_LOCAL(memcpy_orig)
 	retq
 SYM_FUNC_END(memcpy_orig)
 
+.popsection
+
 #ifndef CONFIG_UML
 
 MCSAFE_TEST_CTL

[tip: x86/entry] objtool: Don't consider vmlinux a C-file

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/entry branch of tip:

Commit-ID:     734d099ba644f5a92c70efa3d54d0ba2500ce162
Gitweb:        https://git.kernel.org/tip/734d099ba644f5a92c70efa3d54d0ba2500ce162
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 17 Jun 2020 18:22:31 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 25 Jun 2020 13:45:39 +02:00

objtool: Don't consider vmlinux a C-file

Avoids issuing C-file warnings for vmlinux.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200618144801.701257527@infradead.org
---
 tools/objtool/check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 3e214f8..d8eaa7d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2772,7 +2772,7 @@ int check(const char *_objname, bool orc)
 
 	INIT_LIST_HEAD(&file.insn_list);
 	hash_init(file.insn_hash);
-	file.c_file = find_section_by_name(file.elf, ".comment");
+	file.c_file = !vmlinux && find_section_by_name(file.elf, ".comment");
 	file.ignore_unreachables = no_unreachable;
 	file.hints = false;