[PATCH 00/11] x86/fault: Cleanups and robustifications

[PATCH 00/11] x86/fault: Cleanups and robustifications

[Andy Lutomirski]

The BPF team reported a warning in the x86 page fault code.  This caused me
to read said code, and it was quite tangled.  This series attempts to
mostly disentangle it and fixes a whole bunch of corner cases.  In my
opinion, the control flow and the semantics of the various page fault
functions are much clearer with this series applied, and it also fixes
warts in the SMEP, SMAP, and WRUSS corners of the page fault handler.

Fortunately we don't have any WRUSS instructions in the kernel yet, but I
can dream that some day we'll get real instructions for explicit access to
normal user memory, in which case they will also benefit from these fixes.

The first patch is a genuine regression fix, but the rest is potentially
subtle enough that I would like it to have a good long soak in -next before
landing.

As a nice side bonus, the first patch should give a decent speedup to page
fault signal delivery on CPUs that are not affected by AMD erratum #91.

Andy Lutomirski (11):
  x86/fault: Fix AMD erratum #91 errata fixup for user code
  x86/fault: Fold mm_fault_error() into do_user_addr_fault()
  x86/fault/32: Move is_f00f_bug() do do_kern_addr_fault()
  x86/fault: Document the locking in the fault_signal_pending() path
  x86/fault: Correct a few user vs kernel checks wrt WRUSS
  x86/fault: Improve kernel-executing-user-memory handling
  x86/fault: Split the OOPS code out from no_context()
  x86/fault: Bypass no_context() for implicit kernel faults from
    usermode
  x86/fault: Rename no_context() to kernelmode_fixup_or_oops()
  x86/fault: Don't run fixups for SMAP violations
  x86/fault: Don't look for extable entries for SMEP violations

 arch/x86/mm/fault.c | 355 ++++++++++++++++++++++++--------------------
 1 file changed, 197 insertions(+), 158 deletions(-)

-- 
2.29.2

[PATCH 01/11] x86/fault: Fix AMD erratum #91 errata fixup for user code

[Andy Lutomirski]

The recent rework of probe_kernel_read() and its conversion to
get_kernel_nofault() inadvertently broke is_prefetch().  We were using
probe_kernel_read() as a sloppy "read user or kernel memory" helper, but it
doens't do that any more.  The new get_kernel_nofault() reads *kernel*
memory only, which completely broke is_prefetch() for user access.

Adjust the code to the the correct accessor based on access mode.  The
manual address bounds check is no longer necessary, since the accessor
helpers (get_user() / get_kernel_nofault()) do the right thing all by
themselves.  As a bonus, by using the correct accessor, we don't need the
open-coded address bounds check.

While we're at it, disable the workaround on all CPUs except AMD Family
0xF.  By my reading of the Revision Guide for AMD Athlon™ 64 and AMD
Opteron™ Processors, only family 0xF is affected.

Fixes: eab0c6089b68 ("maccess: unify the probe kernel arch hooks")
Cc: stable@vger.kernel.org
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 106b22d1d189..50dfdc71761e 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -54,7 +54,7 @@ kmmio_fault(struct pt_regs *regs, unsigned long addr)
  * 32-bit mode:
  *
  *   Sometimes AMD Athlon/Opteron CPUs report invalid exceptions on prefetch.
- *   Check that here and ignore it.
+ *   Check that here and ignore it.  This is AMD erratum #91.
  *
  * 64-bit mode:
  *
@@ -83,11 +83,7 @@ check_prefetch_opcode(struct pt_regs *regs, unsigned char *instr,
 #ifdef CONFIG_X86_64
 	case 0x40:
 		/*
-		 * In AMD64 long mode 0x40..0x4F are valid REX prefixes
-		 * Need to figure out under what instruction mode the
-		 * instruction was issued. Could check the LDT for lm,
-		 * but for now it's good enough to assume that long
-		 * mode only uses well known segments or kernel.
+		 * In 64-bit mode 0x40..0x4F are valid REX prefixes
 		 */
 		return (!user_mode(regs) || user_64bit_mode(regs));
 #endif
@@ -124,23 +120,38 @@ is_prefetch(struct pt_regs *regs, unsigned long error_code, unsigned long addr)
 	if (error_code & X86_PF_INSTR)
 		return 0;
 
+	if (likely(boot_cpu_data.x86_vendor != X86_VENDOR_AMD
+		   || boot_cpu_data.x86 != 0xf))
+		return 0;
+
 	instr = (void *)convert_ip_to_linear(current, regs);
 	max_instr = instr + 15;
 
-	if (user_mode(regs) && instr >= (unsigned char *)TASK_SIZE_MAX)
-		return 0;
+	/*
+	 * This code has historically always bailed out if IP points to a
+	 * not-present page (e.g. due to a race).  No one has ever
+	 * complained about this.
+	 */
+	pagefault_disable();
 
 	while (instr < max_instr) {
 		unsigned char opcode;
 
-		if (get_kernel_nofault(opcode, instr))
-			break;
+		if (user_mode(regs)) {
+			if (get_user(opcode, instr))
+				break;
+		} else {
+			if (get_kernel_nofault(opcode, instr))
+				break;
+		}
 
 		instr++;
 
 		if (!check_prefetch_opcode(regs, instr, opcode, &prefetch))
 			break;
 	}
+
+	pagefault_enable();
 	return prefetch;
 }
 
-- 
2.29.2

[PATCH 02/11] x86/fault: Fold mm_fault_error() into do_user_addr_fault()

[Andy Lutomirski]

mm_fault_error() is logically just the end of do_user_addr_fault().
Combine the functions.  This makes the code easier to read.

Most of the churn here is from renaming hw_error_code to error_code in
do_user_addr_fault().

This makes no difference at all to the generated code (objdump -dr) as
compared to changing noinline to __always_inline in the definition of
mm_fault_error().

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 97 +++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 52 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 50dfdc71761e..aff35c9ba018 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -944,40 +944,6 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 	force_sig_fault(SIGBUS, BUS_ADRERR, (void __user *)address);
 }
 
-static noinline void
-mm_fault_error(struct pt_regs *regs, unsigned long error_code,
-	       unsigned long address, vm_fault_t fault)
-{
-	if (fatal_signal_pending(current) && !(error_code & X86_PF_USER)) {
-		no_context(regs, error_code, address, 0, 0);
-		return;
-	}
-
-	if (fault & VM_FAULT_OOM) {
-		/* Kernel mode? Handle exceptions or die: */
-		if (!(error_code & X86_PF_USER)) {
-			no_context(regs, error_code, address,
-				   SIGSEGV, SEGV_MAPERR);
-			return;
-		}
-
-		/*
-		 * We ran out of memory, call the OOM killer, and return the
-		 * userspace (which will retry the fault, or kill us if we got
-		 * oom-killed):
-		 */
-		pagefault_out_of_memory();
-	} else {
-		if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
-			     VM_FAULT_HWPOISON_LARGE))
-			do_sigbus(regs, error_code, address, fault);
-		else if (fault & VM_FAULT_SIGSEGV)
-			bad_area_nosemaphore(regs, error_code, address);
-		else
-			BUG();
-	}
-}
-
 static int spurious_kernel_fault_check(unsigned long error_code, pte_t *pte)
 {
 	if ((error_code & X86_PF_WRITE) && !pte_write(*pte))
@@ -1215,7 +1181,7 @@ NOKPROBE_SYMBOL(do_kern_addr_fault);
 /* Handle faults in the user portion of the address space */
 static inline
 void do_user_addr_fault(struct pt_regs *regs,
-			unsigned long hw_error_code,
+			unsigned long error_code,
 			unsigned long address)
 {
 	struct vm_area_struct *vma;
@@ -1235,8 +1201,8 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 * Reserved bits are never expected to be set on
 	 * entries in the user portion of the page tables.
 	 */
-	if (unlikely(hw_error_code & X86_PF_RSVD))
-		pgtable_bad(regs, hw_error_code, address);
+	if (unlikely(error_code & X86_PF_RSVD))
+		pgtable_bad(regs, error_code, address);
 
 	/*
 	 * If SMAP is on, check for invalid kernel (supervisor) access to user
@@ -1246,10 +1212,10 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 * enforcement appears to be consistent with the USER bit.
 	 */
 	if (unlikely(cpu_feature_enabled(X86_FEATURE_SMAP) &&
-		     !(hw_error_code & X86_PF_USER) &&
+		     !(error_code & X86_PF_USER) &&
 		     !(regs->flags & X86_EFLAGS_AC)))
 	{
-		bad_area_nosemaphore(regs, hw_error_code, address);
+		bad_area_nosemaphore(regs, error_code, address);
 		return;
 	}
 
@@ -1258,7 +1224,7 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 * in a region with pagefaults disabled then we must not take the fault
 	 */
 	if (unlikely(faulthandler_disabled() || !mm)) {
-		bad_area_nosemaphore(regs, hw_error_code, address);
+		bad_area_nosemaphore(regs, error_code, address);
 		return;
 	}
 
@@ -1279,9 +1245,9 @@ void do_user_addr_fault(struct pt_regs *regs,
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
 
-	if (hw_error_code & X86_PF_WRITE)
+	if (error_code & X86_PF_WRITE)
 		flags |= FAULT_FLAG_WRITE;
-	if (hw_error_code & X86_PF_INSTR)
+	if (error_code & X86_PF_INSTR)
 		flags |= FAULT_FLAG_INSTRUCTION;
 
 #ifdef CONFIG_X86_64
@@ -1297,7 +1263,7 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 * to consider the PF_PK bit.
 	 */
 	if (is_vsyscall_vaddr(address)) {
-		if (emulate_vsyscall(hw_error_code, regs, address))
+		if (emulate_vsyscall(error_code, regs, address))
 			return;
 	}
 #endif
@@ -1320,7 +1286,7 @@ void do_user_addr_fault(struct pt_regs *regs,
 			 * Fault from code in kernel from
 			 * which we do not expect faults.
 			 */
-			bad_area_nosemaphore(regs, hw_error_code, address);
+			bad_area_nosemaphore(regs, error_code, address);
 			return;
 		}
 retry:
@@ -1336,17 +1302,17 @@ void do_user_addr_fault(struct pt_regs *regs,
 
 	vma = find_vma(mm, address);
 	if (unlikely(!vma)) {
-		bad_area(regs, hw_error_code, address);
+		bad_area(regs, error_code, address);
 		return;
 	}
 	if (likely(vma->vm_start <= address))
 		goto good_area;
 	if (unlikely(!(vma->vm_flags & VM_GROWSDOWN))) {
-		bad_area(regs, hw_error_code, address);
+		bad_area(regs, error_code, address);
 		return;
 	}
 	if (unlikely(expand_stack(vma, address))) {
-		bad_area(regs, hw_error_code, address);
+		bad_area(regs, error_code, address);
 		return;
 	}
 
@@ -1355,8 +1321,8 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 * we can handle it..
 	 */
 good_area:
-	if (unlikely(access_error(hw_error_code, vma))) {
-		bad_area_access_error(regs, hw_error_code, address, vma);
+	if (unlikely(access_error(error_code, vma))) {
+		bad_area_access_error(regs, error_code, address, vma);
 		return;
 	}
 
@@ -1378,7 +1344,7 @@ void do_user_addr_fault(struct pt_regs *regs,
 	/* Quick path to respond to signals */
 	if (fault_signal_pending(fault, regs)) {
 		if (!user_mode(regs))
-			no_context(regs, hw_error_code, address, SIGBUS,
+			no_context(regs, error_code, address, SIGBUS,
 				   BUS_ADRERR);
 		return;
 	}
@@ -1395,10 +1361,37 @@ void do_user_addr_fault(struct pt_regs *regs,
 	}
 
 	mmap_read_unlock(mm);
-	if (unlikely(fault & VM_FAULT_ERROR)) {
-		mm_fault_error(regs, hw_error_code, address, fault);
+	if (likely(!(fault & VM_FAULT_ERROR)))
+		return;
+
+	if (fatal_signal_pending(current) && !(error_code & X86_PF_USER)) {
+		no_context(regs, error_code, address, 0, 0);
 		return;
 	}
+
+	if (fault & VM_FAULT_OOM) {
+		/* Kernel mode? Handle exceptions or die: */
+		if (!(error_code & X86_PF_USER)) {
+			no_context(regs, error_code, address,
+				   SIGSEGV, SEGV_MAPERR);
+			return;
+		}
+
+		/*
+		 * We ran out of memory, call the OOM killer, and return the
+		 * userspace (which will retry the fault, or kill us if we got
+		 * oom-killed):
+		 */
+		pagefault_out_of_memory();
+	} else {
+		if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
+			     VM_FAULT_HWPOISON_LARGE))
+			do_sigbus(regs, error_code, address, fault);
+		else if (fault & VM_FAULT_SIGSEGV)
+			bad_area_nosemaphore(regs, error_code, address);
+		else
+			BUG();
+	}
 }
 NOKPROBE_SYMBOL(do_user_addr_fault);
 
-- 
2.29.2

[PATCH 03/11] x86/fault/32: Move is_f00f_bug() do do_kern_addr_fault()

[Andy Lutomirski]

bad_area() and its relatives are called from many places in fault.c, and
exactly one of them wants the F00F workaround.

__bad_area_nosemaphore() no longer contains any kernel fault code, which
prepares for further cleanups.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index aff35c9ba018..87d30daa5e0a 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -445,10 +445,12 @@ static int is_errata100(struct pt_regs *regs, unsigned long address)
 }
 
 /* Pentium F0 0F C7 C8 bug workaround: */
-static int is_f00f_bug(struct pt_regs *regs, unsigned long address)
+static int is_f00f_bug(struct pt_regs *regs, unsigned long error_code,
+		       unsigned long address)
 {
 #ifdef CONFIG_X86_F00F_BUG
-	if (boot_cpu_has_bug(X86_BUG_F00F) && idt_is_f00f_address(address)) {
+	if (boot_cpu_has_bug(X86_BUG_F00F) && !(error_code & X86_PF_USER) &&
+	    idt_is_f00f_address(address)) {
 		handle_invalid_op(regs);
 		return 1;
 	}
@@ -816,9 +818,6 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		return;
 	}
 
-	if (is_f00f_bug(regs, address))
-		return;
-
 	no_context(regs, error_code, address, SIGSEGV, si_code);
 }
 
@@ -1158,6 +1157,9 @@ do_kern_addr_fault(struct pt_regs *regs, unsigned long hw_error_code,
 	}
 #endif
 
+	if (is_f00f_bug(regs, hw_error_code, address))
+		return;
+
 	/* Was the fault spurious, caused by lazy TLB invalidation? */
 	if (spurious_kernel_fault(hw_error_code, address))
 		return;
-- 
2.29.2

[PATCH 04/11] x86/fault: Document the locking in the fault_signal_pending() path

[Andy Lutomirski]

If fault_signal_pending() returns true, then the core mm has unlocked the
mm for us.  Add a comment to help future readers of this code.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 87d30daa5e0a..b52064920f0d 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1343,8 +1343,11 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 */
 	fault = handle_mm_fault(vma, address, flags, regs);
 
-	/* Quick path to respond to signals */
 	if (fault_signal_pending(fault, regs)) {
+		/*
+		 * Quick path to respond to signals.  The core mm code
+		 * has unlocked the mm for us if we get here.
+		 */
 		if (!user_mode(regs))
 			no_context(regs, error_code, address, SIGBUS,
 				   BUS_ADRERR);
-- 
2.29.2

[PATCH 05/11] x86/fault: Correct a few user vs kernel checks wrt WRUSS

[Andy Lutomirski]

In general, page fault errors for WRUSS should be just like get_user(),
etc.  Fix three bugs in this area:

We have a comment that says that, if we can't handle a page fault on a user
address due to OOM, we will skip the OOM-kill-and-retry logic.  The code
checked kernel *privilege*, not kernel mode, so it missed WRUSS.  This
means that we would malfunction if we got OOM on a WRUSS fault -- this
would be a kernel-mode, user-privilege fault, and we would invoke the OOM
killer and retry.

A failed user access from kernel while a fatal signal is pending should
fail even if the instruction in question was WRUSS.

do_sigbus() should not send SIGBUS for WRUSS -- it should handle it like
any other kernel mode failure.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index b52064920f0d..602cdf8e070a 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -908,7 +908,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 	  vm_fault_t fault)
 {
 	/* Kernel mode? Handle exceptions or die: */
-	if (!(error_code & X86_PF_USER)) {
+	if (!user_mode(regs)) {
 		no_context(regs, error_code, address, SIGBUS, BUS_ADRERR);
 		return;
 	}
@@ -1180,7 +1180,14 @@ do_kern_addr_fault(struct pt_regs *regs, unsigned long hw_error_code,
 }
 NOKPROBE_SYMBOL(do_kern_addr_fault);
 
-/* Handle faults in the user portion of the address space */
+/*
+ * Handle faults in the user portion of the address space.  Nothing in here
+ * should check X86_PF_USER without a specific justification: for almost
+ * all purposes, we should treat a normal kernel access to user memory
+ * (e.g. get_user(), put_user(), etc.) the same as the WRUSS instruction.
+ * The one exception is AC flag handling, which is, per the x86
+ * architecture, special for WRUSS.
+ */
 static inline
 void do_user_addr_fault(struct pt_regs *regs,
 			unsigned long error_code,
@@ -1369,14 +1376,14 @@ void do_user_addr_fault(struct pt_regs *regs,
 	if (likely(!(fault & VM_FAULT_ERROR)))
 		return;
 
-	if (fatal_signal_pending(current) && !(error_code & X86_PF_USER)) {
+	if (fatal_signal_pending(current) && !user_mode(regs)) {
 		no_context(regs, error_code, address, 0, 0);
 		return;
 	}
 
 	if (fault & VM_FAULT_OOM) {
 		/* Kernel mode? Handle exceptions or die: */
-		if (!(error_code & X86_PF_USER)) {
+		if (!user_mode(regs)) {
 			no_context(regs, error_code, address,
 				   SIGSEGV, SEGV_MAPERR);
 			return;
-- 
2.29.2

[PATCH 06/11] x86/fault: Improve kernel-executing-user-memory handling

[Andy Lutomirski]

Right now we treat the case of the kernel trying to execute from user
memory more or less just like the kernel getting a page fault on a user
access.  In the failure path, we check for erratum #93, try to otherwise
fix up the error, and then oops.

If we manage to jump to the user address space, with or without SMEP, we
should not try to resolve the page fault.  This is an error, pure and
simple.  Rearrange the code so that we catch this case early, check for
erratum #93, and bail out.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 602cdf8e070a..1939e546beae 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -406,8 +406,11 @@ static void dump_pagetable(unsigned long address)
 static int is_errata93(struct pt_regs *regs, unsigned long address)
 {
 #if defined(CONFIG_X86_64) && defined(CONFIG_CPU_SUP_AMD)
-	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD
-	    || boot_cpu_data.x86 != 0xf)
+	if (likely(boot_cpu_data.x86_vendor != X86_VENDOR_AMD
+		   || boot_cpu_data.x86 != 0xf))
+		return 0;
+
+	if (user_mode(regs))
 		return 0;
 
 	if (address != regs->ip)
@@ -707,9 +710,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	if (is_prefetch(regs, error_code, address))
 		return;
 
-	if (is_errata93(regs, address))
-		return;
-
 	/*
 	 * Buggy firmware could access regions which might page fault, try to
 	 * recover from such faults.
@@ -1202,6 +1202,19 @@ void do_user_addr_fault(struct pt_regs *regs,
 	tsk = current;
 	mm = tsk->mm;
 
+	if (unlikely((error_code & (X86_PF_USER | X86_PF_INSTR)) == X86_PF_INSTR)) {
+		/*
+		 * Whoops, this is kernel mode code trying to execute from
+		 * user memory.  Unless this is AMD erratum #93, we are toast.
+		 * Don't even try to look up the VMA.
+		 */
+		if (is_errata93(regs, address))
+			return;
+
+		bad_area_nosemaphore(regs, error_code, address);
+		return;
+	}
+
 	/* kprobes don't want to hook the spurious faults: */
 	if (unlikely(kprobe_page_fault(regs, X86_TRAP_PF)))
 		return;
-- 
2.29.2

[PATCH 07/11] x86/fault: Split the OOPS code out from no_context()

[Andy Lutomirski]

Not all callers of no_context() want to run exception fixups.
Separate the OOPS code out from the fixup code in no_context().

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 116 +++++++++++++++++++++++---------------------
 1 file changed, 62 insertions(+), 54 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 1939e546beae..6f43d080e1e8 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -618,53 +618,20 @@ static void set_signal_archinfo(unsigned long address,
 }
 
 static noinline void
-no_context(struct pt_regs *regs, unsigned long error_code,
-	   unsigned long address, int signal, int si_code)
+page_fault_oops(struct pt_regs *regs, unsigned long error_code,
+		unsigned long address)
 {
-	struct task_struct *tsk = current;
 	unsigned long flags;
 	int sig;
 
 	if (user_mode(regs)) {
 		/*
-		 * This is an implicit supervisor-mode access from user
-		 * mode.  Bypass all the kernel-mode recovery code and just
-		 * OOPS.
+		 * Implicit kernel access from user mode?  Skip the stack
+		 * overflow and EFI special cases.
 		 */
 		goto oops;
 	}
 
-	/* Are we prepared to handle this kernel fault? */
-	if (fixup_exception(regs, X86_TRAP_PF, error_code, address)) {
-		/*
-		 * Any interrupt that takes a fault gets the fixup. This makes
-		 * the below recursive fault logic only apply to a faults from
-		 * task context.
-		 */
-		if (in_interrupt())
-			return;
-
-		/*
-		 * Per the above we're !in_interrupt(), aka. task context.
-		 *
-		 * In this case we need to make sure we're not recursively
-		 * faulting through the emulate_vsyscall() logic.
-		 */
-		if (current->thread.sig_on_uaccess_err && signal) {
-			sanitize_error_code(address, &error_code);
-
-			set_signal_archinfo(address, error_code);
-
-			/* XXX: hwpoison faults will set the wrong code. */
-			force_sig_fault(signal, si_code, (void __user *)address);
-		}
-
-		/*
-		 * Barring that, we can do the fixup and be happy.
-		 */
-		return;
-	}
-
 #ifdef CONFIG_VMAP_STACK
 	/*
 	 * Stack overflow?  During boot, we can fault near the initial
@@ -672,8 +639,8 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	 * that we're in vmalloc space to avoid this.
 	 */
 	if (is_vmalloc_addr((void *)address) &&
-	    (((unsigned long)tsk->stack - 1 - address < PAGE_SIZE) ||
-	     address - ((unsigned long)tsk->stack + THREAD_SIZE) < PAGE_SIZE)) {
+	    (((unsigned long)current->stack - 1 - address < PAGE_SIZE) ||
+	     address - ((unsigned long)current->stack + THREAD_SIZE) < PAGE_SIZE)) {
 		unsigned long stack = __this_cpu_ist_top_va(DF) - sizeof(void *);
 		/*
 		 * We're likely to be running with very little stack space
@@ -696,20 +663,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	}
 #endif
 
-	/*
-	 * 32-bit:
-	 *
-	 *   Valid to do another page fault here, because if this fault
-	 *   had been triggered by is_prefetch fixup_exception would have
-	 *   handled it.
-	 *
-	 * 64-bit:
-	 *
-	 *   Hall of shame of CPU/BIOS bugs.
-	 */
-	if (is_prefetch(regs, error_code, address))
-		return;
-
 	/*
 	 * Buggy firmware could access regions which might page fault, try to
 	 * recover from such faults.
@@ -726,7 +679,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 
 	show_fault_oops(regs, error_code, address);
 
-	if (task_stack_end_corrupted(tsk))
+	if (task_stack_end_corrupted(current))
 		printk(KERN_EMERG "Thread overran stack, or stack corrupted\n");
 
 	sig = SIGKILL;
@@ -739,6 +692,61 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	oops_end(flags, regs, sig);
 }
 
+static noinline void
+no_context(struct pt_regs *regs, unsigned long error_code,
+	   unsigned long address, int signal, int si_code)
+{
+	if (user_mode(regs)) {
+		/*
+		 * This is an implicit supervisor-mode access from user
+		 * mode.  Bypass all the kernel-mode recovery code and just
+		 * OOPS.
+		 */
+		goto oops;
+	}
+
+	/* Are we prepared to handle this kernel fault? */
+	if (fixup_exception(regs, X86_TRAP_PF, error_code, address)) {
+		/*
+		 * Any interrupt that takes a fault gets the fixup. This makes
+		 * the below recursive fault logic only apply to a faults from
+		 * task context.
+		 */
+		if (in_interrupt())
+			return;
+
+		/*
+		 * Per the above we're !in_interrupt(), aka. task context.
+		 *
+		 * In this case we need to make sure we're not recursively
+		 * faulting through the emulate_vsyscall() logic.
+		 */
+		if (current->thread.sig_on_uaccess_err && signal) {
+			sanitize_error_code(address, &error_code);
+
+			set_signal_archinfo(address, error_code);
+
+			/* XXX: hwpoison faults will set the wrong code. */
+			force_sig_fault(signal, si_code, (void __user *)address);
+		}
+
+		/*
+		 * Barring that, we can do the fixup and be happy.
+		 */
+		return;
+	}
+
+	/*
+	 * AMD erratum #91 manifests as a spurious page fault on a PREFETCH
+	 * instruction.
+	 */
+	if (is_prefetch(regs, error_code, address))
+		return;
+
+oops:
+	page_fault_oops(regs, error_code, address);
+}
+
 /*
  * Print out info about fatal segfaults, if the show_unhandled_signals
  * sysctl is set:
-- 
2.29.2

[PATCH 08/11] x86/fault: Bypass no_context() for implicit kernel faults from usermode

[Andy Lutomirski]

We can drop an indentation level and remove the last user_mode(regs) == true
caller of no_context() by directly OOPSing for implicit kernel faults from
usermode.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 59 ++++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 6f43d080e1e8..177b612c7f33 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -789,44 +789,49 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 {
 	struct task_struct *tsk = current;
 
-	/* User mode accesses just cause a SIGSEGV */
-	if (user_mode(regs) && (error_code & X86_PF_USER)) {
-		/*
-		 * It's possible to have interrupts off here:
-		 */
-		local_irq_enable();
+	if (!user_mode(regs)) {
+		no_context(regs, error_code, address, pkey, si_code);
+		return;
+	}
 
-		/*
-		 * Valid to do another page fault here because this one came
-		 * from user space:
-		 */
-		if (is_prefetch(regs, error_code, address))
-			return;
+	if (!(error_code & X86_PF_USER)) {
+		/* Implicit user access to kernel memory -- just oops */
+		page_fault_oops(regs, error_code, address);
+		return;
+	}
 
-		if (is_errata100(regs, address))
-			return;
+	/*
+	 * User mode accesses just cause a SIGSEGV.
+	 * It's possible to have interrupts off here:
+	 */
+	local_irq_enable();
 
-		sanitize_error_code(address, &error_code);
+	/*
+	 * Valid to do another page fault here because this one came
+	 * from user space:
+	 */
+	if (is_prefetch(regs, error_code, address))
+		return;
 
-		if (fixup_vdso_exception(regs, X86_TRAP_PF, error_code, address))
-			return;
+	if (is_errata100(regs, address))
+		return;
 
-		if (likely(show_unhandled_signals))
-			show_signal_msg(regs, error_code, address, tsk);
+	sanitize_error_code(address, &error_code);
 
-		set_signal_archinfo(address, error_code);
+	if (fixup_vdso_exception(regs, X86_TRAP_PF, error_code, address))
+		return;
 
-		if (si_code == SEGV_PKUERR)
-			force_sig_pkuerr((void __user *)address, pkey);
+	if (likely(show_unhandled_signals))
+		show_signal_msg(regs, error_code, address, tsk);
 
-		force_sig_fault(SIGSEGV, si_code, (void __user *)address);
+	set_signal_archinfo(address, error_code);
 
-		local_irq_disable();
+	if (si_code == SEGV_PKUERR)
+		force_sig_pkuerr((void __user *)address, pkey);
 
-		return;
-	}
+	force_sig_fault(SIGSEGV, si_code, (void __user *)address);
 
-	no_context(regs, error_code, address, SIGSEGV, si_code);
+	local_irq_disable();
 }
 
 static noinline void
-- 
2.29.2

[PATCH 09/11] x86/fault: Rename no_context() to kernelmode_fixup_or_oops()

[Andy Lutomirski]

The name no_context() has never been very clear.  It's only called for
faults from kernel mode, so rename it and change the no-longer-useful
user_mode(regs) check to a WARN_ON_ONCE.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 177b612c7f33..04cc98ec2423 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -693,17 +693,10 @@ page_fault_oops(struct pt_regs *regs, unsigned long error_code,
 }
 
 static noinline void
-no_context(struct pt_regs *regs, unsigned long error_code,
-	   unsigned long address, int signal, int si_code)
+kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
+			 unsigned long address, int signal, int si_code)
 {
-	if (user_mode(regs)) {
-		/*
-		 * This is an implicit supervisor-mode access from user
-		 * mode.  Bypass all the kernel-mode recovery code and just
-		 * OOPS.
-		 */
-		goto oops;
-	}
+	WARN_ON_ONCE(user_mode(regs));
 
 	/* Are we prepared to handle this kernel fault? */
 	if (fixup_exception(regs, X86_TRAP_PF, error_code, address)) {
@@ -743,7 +736,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	if (is_prefetch(regs, error_code, address))
 		return;
 
-oops:
 	page_fault_oops(regs, error_code, address);
 }
 
@@ -790,7 +782,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 	struct task_struct *tsk = current;
 
 	if (!user_mode(regs)) {
-		no_context(regs, error_code, address, pkey, si_code);
+		kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code);
 		return;
 	}
 
@@ -922,7 +914,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 {
 	/* Kernel mode? Handle exceptions or die: */
 	if (!user_mode(regs)) {
-		no_context(regs, error_code, address, SIGBUS, BUS_ADRERR);
+		kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR);
 		return;
 	}
 
@@ -1382,8 +1374,8 @@ void do_user_addr_fault(struct pt_regs *regs,
 		 * has unlocked the mm for us if we get here.
 		 */
 		if (!user_mode(regs))
-			no_context(regs, error_code, address, SIGBUS,
-				   BUS_ADRERR);
+			kernelmode_fixup_or_oops(regs, error_code, address,
+						 SIGBUS, BUS_ADRERR);
 		return;
 	}
 
@@ -1403,15 +1395,15 @@ void do_user_addr_fault(struct pt_regs *regs,
 		return;
 
 	if (fatal_signal_pending(current) && !user_mode(regs)) {
-		no_context(regs, error_code, address, 0, 0);
+		kernelmode_fixup_or_oops(regs, error_code, address, 0, 0);
 		return;
 	}
 
 	if (fault & VM_FAULT_OOM) {
 		/* Kernel mode? Handle exceptions or die: */
 		if (!user_mode(regs)) {
-			no_context(regs, error_code, address,
-				   SIGSEGV, SEGV_MAPERR);
+			kernelmode_fixup_or_oops(regs, error_code, address,
+						 SIGSEGV, SEGV_MAPERR);
 			return;
 		}
 
-- 
2.29.2

[PATCH 10/11] x86/fault: Don't run fixups for SMAP violations

[Andy Lutomirski]

A SMAP-violating kernel access is not a recoverable condition.  Imagine
kernel code that, outside of a uaccess region, dereferences a pointer to
the user range by accident.  If SMAP is on, this will reliably generate
as an intentional user access.  This makes it easy for bugs to be
overlooked if code is inadequately tested both with and without SMAP.

We discovered this because BPF can generate invalid accesses to user
memory, but those warnings only got printed if SMAP was off.  With this
patch, this type of error will be discovered with SMAP on as well.

Cc: Yonghong Song <yhs@fb.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 04cc98ec2423..d39946ad8a91 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1242,7 +1242,11 @@ void do_user_addr_fault(struct pt_regs *regs,
 		     !(error_code & X86_PF_USER) &&
 		     !(regs->flags & X86_EFLAGS_AC)))
 	{
-		bad_area_nosemaphore(regs, error_code, address);
+		/*
+		 * No extable entry here.  This was a kernel access to an
+		 * invalid pointer.  get_kernel_nofault() will not get here.
+		 */
+		page_fault_oops(regs, error_code, address);
 		return;
 	}
 
-- 
2.29.2

[PATCH 11/11] x86/fault: Don't look for extable entries for SMEP violations

[Andy Lutomirski]

If we get a SMEP violation or a fault that would have been a SMEP
violation if we had SMEP, we shouldn't run fixups.  Just OOPS.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/fault.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index d39946ad8a91..08f5f74cf989 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1211,12 +1211,13 @@ void do_user_addr_fault(struct pt_regs *regs,
 		/*
 		 * Whoops, this is kernel mode code trying to execute from
 		 * user memory.  Unless this is AMD erratum #93, we are toast.
-		 * Don't even try to look up the VMA.
+		 * Don't even try to look up the VMA or look for extable
+		 * entries.
 		 */
 		if (is_errata93(regs, address))
 			return;
 
-		bad_area_nosemaphore(regs, error_code, address);
+		page_fault_oops(regs, error_code, address);
 		return;
 	}
 
-- 
2.29.2

Re: [PATCH 01/11] x86/fault: Fix AMD erratum #91 errata fixup for user code

[Christoph Hellwig]

On Sun, Jan 31, 2021 at 09:24:32AM -0800, Andy Lutomirski wrote:
> While we're at it, disable the workaround on all CPUs except AMD Family
> 0xF.  By my reading of the Revision Guide for AMD Athlon™ 64 and AMD
> Opteron™ Processors, only family 0xF is affected.

I think it would be better to have one no risk refression fix that
just probes both user and kernel addresses and a separate one to
restrict the workaround.

> +	if (likely(boot_cpu_data.x86_vendor != X86_VENDOR_AMD
> +		   || boot_cpu_data.x86 != 0xf))

Normally kernel style would be to have the || on the first line.

Re: [PATCH 06/11] x86/fault: Improve kernel-executing-user-memory handling

[Christoph Hellwig]

On Sun, Jan 31, 2021 at 09:24:37AM -0800, Andy Lutomirski wrote:
>  #if defined(CONFIG_X86_64) && defined(CONFIG_CPU_SUP_AMD)
> +	if (likely(boot_cpu_data.x86_vendor != X86_VENDOR_AMD
> +		   || boot_cpu_data.x86 != 0xf))

Same nitpick as for the other patch.  Maybe we wan a little inline
helper for the specific erratum that includes the vendor and family
checks in adddition to using IS_ENABLED for the config options?

Re: [PATCH 09/11] x86/fault: Rename no_context() to kernelmode_fixup_or_oops()

[Christoph Hellwig]

On Sun, Jan 31, 2021 at 09:24:40AM -0800, Andy Lutomirski wrote:
> +		kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code);

>  	if (!user_mode(regs)) {
> -		no_context(regs, error_code, address, SIGBUS, BUS_ADRERR);
> +		kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR);

These overly long lines are a little annoying..

Re: [PATCH 01/11] x86/fault: Fix AMD erratum #91 errata fixup for user code

[Borislav Petkov]

On Sun, Jan 31, 2021 at 09:24:32AM -0800, Andy Lutomirski wrote:
> The recent rework of probe_kernel_read() and its conversion to

Judging by

  25f12ae45fc1 ("maccess: rename probe_kernel_address to get_kernel_nofault")

I think you mean probe_kernel_address() above and below.

> get_kernel_nofault() inadvertently broke is_prefetch().  We were using

Let's drop the "we" pls and switch to passive voice.

> probe_kernel_read() as a sloppy "read user or kernel memory" helper, but it
> doens't do that any more.  The new get_kernel_nofault() reads *kernel*
> memory only, which completely broke is_prefetch() for user access.
> 
> Adjust the code to the the correct accessor based on access mode.  The

s/the //

> manual address bounds check is no longer necessary, since the accessor
> helpers (get_user() / get_kernel_nofault()) do the right thing all by
> themselves.  As a bonus, by using the correct accessor, we don't need the
> open-coded address bounds check.
> 
> While we're at it, disable the workaround on all CPUs except AMD Family
> 0xF.  By my reading of the Revision Guide for AMD Athlon™ 64 and AMD
> Opteron™ Processors, only family 0xF is affected.

Yah, actually, only !NPT K8s have the erratum listed, i.e., CPU models <
0x40, AFAICT.

I.e., your test should be:

	struct cpuinfo_x86 *c = &boot_cpu_data;

	...

	/* Erratum #91 on AMD K8, pre-NPT CPUs */
        if (likely(c->x86_vendor != X86_VENDOR_AMD ||
		   c->x86 != 0xf ||
		   c->x86_model >= 0x40))
		return 0;

I can try to dig out such a machine to test this on if you wanna. We
might still have one collecting dust somewhere in a corner...

> Fixes: eab0c6089b68 ("maccess: unify the probe kernel arch hooks")
> Cc: stable@vger.kernel.org

@stable because theoretically without that fix, kernel should explode on
those machines when it #PFs on a prefetch insn in user mode?

Hmm, yap, probably...

> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/mm/fault.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 106b22d1d189..50dfdc71761e 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -54,7 +54,7 @@ kmmio_fault(struct pt_regs *regs, unsigned long addr)
>   * 32-bit mode:
>   *
>   *   Sometimes AMD Athlon/Opteron CPUs report invalid exceptions on prefetch.
> - *   Check that here and ignore it.
> + *   Check that here and ignore it.  This is AMD erratum #91.
>   *
>   * 64-bit mode:
>   *
> @@ -83,11 +83,7 @@ check_prefetch_opcode(struct pt_regs *regs, unsigned char *instr,
>  #ifdef CONFIG_X86_64
>  	case 0x40:
>  		/*
> -		 * In AMD64 long mode 0x40..0x4F are valid REX prefixes
> -		 * Need to figure out under what instruction mode the
> -		 * instruction was issued. Could check the LDT for lm,
> -		 * but for now it's good enough to assume that long
> -		 * mode only uses well known segments or kernel.
> +		 * In 64-bit mode 0x40..0x4F are valid REX prefixes
>  		 */
>  		return (!user_mode(regs) || user_64bit_mode(regs));
>  #endif

Yah, no need to convert that to the insn decoder - that can die together
with the hardware it is supposed to query...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 06/11] x86/fault: Improve kernel-executing-user-memory handling

[Andy Lutomirski]

On Mon, Feb 1, 2021 at 1:08 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sun, Jan 31, 2021 at 09:24:37AM -0800, Andy Lutomirski wrote:
> >  #if defined(CONFIG_X86_64) && defined(CONFIG_CPU_SUP_AMD)
> > +     if (likely(boot_cpu_data.x86_vendor != X86_VENDOR_AMD
> > +                || boot_cpu_data.x86 != 0xf))
>
> Same nitpick as for the other patch.  Maybe we wan a little inline
> helper for the specific erratum that includes the vendor and family
> checks in adddition to using IS_ENABLED for the config options?

I defer to Boris as to exactly what condition we should check here.

Re: [PATCH 09/11] x86/fault: Rename no_context() to kernelmode_fixup_or_oops()

[Andy Lutomirski]

On Mon, Feb 1, 2021 at 1:14 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sun, Jan 31, 2021 at 09:24:40AM -0800, Andy Lutomirski wrote:
> > +             kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code);
>
> >       if (!user_mode(regs)) {
> > -             no_context(regs, error_code, address, SIGBUS, BUS_ADRERR);
> > +             kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR);
>
> These overly long lines are a little annoying..

It's not that long, and Linus seems to think you should make your
terminal wider :)

--Andy

Re: [PATCH 03/11] x86/fault/32: Move is_f00f_bug() do do_kern_addr_fault()

[Borislav Petkov]

On Sun, Jan 31, 2021 at 09:24:34AM -0800, Andy Lutomirski wrote:


> Subject: Re: [PATCH 03/11] x86/fault/32: Move is_f00f_bug() do do_kern_addr_fault()

s/do /to /

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 05/11] x86/fault: Correct a few user vs kernel checks wrt WRUSS

[Borislav Petkov]

On Sun, Jan 31, 2021 at 09:24:36AM -0800, Andy Lutomirski wrote:
> In general, page fault errors for WRUSS should be just like get_user(),
> etc.  Fix three bugs in this area:
> 
> We have a comment that says that, if we can't handle a page fault on a user
> address due to OOM, we will skip the OOM-kill-and-retry logic.  The code
> checked kernel *privilege*, not kernel mode, so it missed WRUSS.  This
> means that we would malfunction if we got OOM on a WRUSS fault -- this
> would be a kernel-mode, user-privilege fault, and we would invoke the OOM
> killer and retry.

Please convert all those commit messages to passive voice - the "we" is
ambiguous.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 06/11] x86/fault: Improve kernel-executing-user-memory handling

[Borislav Petkov]

On Mon, Feb 01, 2021 at 05:00:37PM -0800, Andy Lutomirski wrote:
> I defer to Boris as to exactly what condition we should check here.

Same as for erratum #91:

https://lkml.kernel.org/r/20210201203146.GC14590@zn.tnic

A single helper containing that check is probably a good idea.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 06/11] x86/fault: Improve kernel-executing-user-memory handling

[Borislav Petkov]

On Sun, Jan 31, 2021 at 09:24:37AM -0800, Andy Lutomirski wrote:
> Right now we treat the case of the kernel trying to execute from user
> memory more or less just like the kernel getting a page fault on a user
> access.  In the failure path, we check for erratum #93, try to otherwise
> fix up the error, and then oops.
> 
> If we manage to jump to the user address space, with or without SMEP, we
> should not try to resolve the page fault.  This is an error, pure and
> simple.  Rearrange the code so that we catch this case early, check for
> erratum #93, and bail out.

And I'm scratching my head why are you talking about kernel trying
to execute from user memory but doh, that erratum truncates the high
32-bits of rIP and the #PF address looks like a user address but
X86_PF_USER is clear.

Aha.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 07/11] x86/fault: Split the OOPS code out from no_context()

[Borislav Petkov]

On Sun, Jan 31, 2021 at 09:24:38AM -0800, Andy Lutomirski wrote:
> Not all callers of no_context() want to run exception fixups.
> Separate the OOPS code out from the fixup code in no_context().
> 
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/mm/fault.c | 116 +++++++++++++++++++++++---------------------
>  1 file changed, 62 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 1939e546beae..6f43d080e1e8 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -618,53 +618,20 @@ static void set_signal_archinfo(unsigned long address,
>  }
>  
>  static noinline void
> -no_context(struct pt_regs *regs, unsigned long error_code,
> -	   unsigned long address, int signal, int si_code)
> +page_fault_oops(struct pt_regs *regs, unsigned long error_code,

Not sure about this name - it still tries to recover:
efi_recover_from_page_fault().

Judging by where it is called, maybe no_context_tail() or
no_context_oops() or no_context_finish_me_already()...

Yah, I haz no better idea. :-\

...

> @@ -739,6 +692,61 @@ no_context(struct pt_regs *regs, unsigned long error_code,
>  	oops_end(flags, regs, sig);
>  }
>  
> +static noinline void
> +no_context(struct pt_regs *regs, unsigned long error_code,
> +	   unsigned long address, int signal, int si_code)
> +{
> +	if (user_mode(regs)) {
> +		/*
> +		 * This is an implicit supervisor-mode access from user
> +		 * mode.  Bypass all the kernel-mode recovery code and just
> +		 * OOPS.
> +		 */
> +		goto oops;

Just do

		return page_fault_oops(...);

here and get rid of the label.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 07/11] x86/fault: Split the OOPS code out from no_context()

[Andy Lutomirski]

> On Feb 3, 2021, at 10:56 AM, Borislav Petkov <bp@alien8.de> wrote:
> 
> On Sun, Jan 31, 2021 at 09:24:38AM -0800, Andy Lutomirski wrote:
>> Not all callers of no_context() want to run exception fixups.
>> Separate the OOPS code out from the fixup code in no_context().
>> 
>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>> arch/x86/mm/fault.c | 116 +++++++++++++++++++++++---------------------
>> 1 file changed, 62 insertions(+), 54 deletions(-)
>> 
>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> index 1939e546beae..6f43d080e1e8 100644
>> --- a/arch/x86/mm/fault.c
>> +++ b/arch/x86/mm/fault.c
>> @@ -618,53 +618,20 @@ static void set_signal_archinfo(unsigned long address,
>> }
>> 
>> static noinline void
>> -no_context(struct pt_regs *regs, unsigned long error_code,
>> -       unsigned long address, int signal, int si_code)
>> +page_fault_oops(struct pt_regs *regs, unsigned long error_code,
> 
> Not sure about this name - it still tries to recover:
> efi_recover_from_page_fault().
> 

That function is a lie. It tries to keep the system alive but it doesn’t return if it “recovers”.   Maybe I should add a comment?

> Judging by where it is called, maybe no_context_tail() or
> no_context_oops() or no_context_finish_me_already()...
> 
> Yah, I haz no better idea. :-\
> 
> ...
> 
>> @@ -739,6 +692,61 @@ no_context(struct pt_regs *regs, unsigned long error_code,
>>    oops_end(flags, regs, sig);
>> }
>> 
>> +static noinline void
>> +no_context(struct pt_regs *regs, unsigned long error_code,
>> +       unsigned long address, int signal, int si_code)
>> +{
>> +    if (user_mode(regs)) {
>> +        /*
>> +         * This is an implicit supervisor-mode access from user
>> +         * mode.  Bypass all the kernel-mode recovery code and just
>> +         * OOPS.
>> +         */
>> +        goto oops;
> 
> Just do
> 
>        return page_fault_oops(...);
> 
> here and get rid of the label.
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 09/11] x86/fault: Rename no_context() to kernelmode_fixup_or_oops()

[Borislav Petkov]

On Sun, Jan 31, 2021 at 09:24:40AM -0800, Andy Lutomirski wrote:
> The name no_context() has never been very clear.  It's only called for
> faults from kernel mode, so rename it and change the no-longer-useful
> user_mode(regs) check to a WARN_ON_ONCE.
> 
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/mm/fault.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 177b612c7f33..04cc98ec2423 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -693,17 +693,10 @@ page_fault_oops(struct pt_regs *regs, unsigned long error_code,
>  }
>  
>  static noinline void
> -no_context(struct pt_regs *regs, unsigned long error_code,
> -	   unsigned long address, int signal, int si_code)
> +kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
> +			 unsigned long address, int signal, int si_code)

Ew, I don't like functions with "or" in the name - they're probably not
doing one thing only as they should.

Why not simply "handle_kernel_fault" ?

Also, all the callsites now do:

	if (!user_mode(regs)) {
		kernelmode_fixup_or_oops
		...

I guess you can push the "user_mode" check inside that function for less
hairy code at the callsites.

>  {
> -	if (user_mode(regs)) {
> -		/*
> -		 * This is an implicit supervisor-mode access from user
> -		 * mode.  Bypass all the kernel-mode recovery code and just
> -		 * OOPS.
> -		 */
> -		goto oops;
> -	}
> +	WARN_ON_ONCE(user_mode(regs));

I guess...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 07/11] x86/fault: Split the OOPS code out from no_context()

[Borislav Petkov]

On Wed, Feb 03, 2021 at 11:29:39AM -0800, Andy Lutomirski wrote:
> That function is a lie. It tries to keep the system alive but it
> doesn’t return if it “recovers”. Maybe I should add a comment?

... or rename it?

That schedule() at the end, I dunno if that does anything since we're
going to oops anyway...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 10/11] x86/fault: Don't run fixups for SMAP violations

[Borislav Petkov]

On Sun, Jan 31, 2021 at 09:24:41AM -0800, Andy Lutomirski wrote:
> A SMAP-violating kernel access is not a recoverable condition.  Imagine
> kernel code that, outside of a uaccess region, dereferences a pointer to
> the user range by accident.  If SMAP is on, this will reliably generate
> as an intentional user access.  This makes it easy for bugs to be
> overlooked if code is inadequately tested both with and without SMAP.
> 
> We discovered this because BPF can generate invalid accesses to user
> memory, but those warnings only got printed if SMAP was off.  With this
> patch, this type of error will be discovered with SMAP on as well.
> 
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/mm/fault.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 04cc98ec2423..d39946ad8a91 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1242,7 +1242,11 @@ void do_user_addr_fault(struct pt_regs *regs,
>  		     !(error_code & X86_PF_USER) &&
>  		     !(regs->flags & X86_EFLAGS_AC)))
>  	{
	^

Might wanna fix that opening brace too.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 09/11] x86/fault: Rename no_context() to kernelmode_fixup_or_oops()

[Andy Lutomirski]

> On Feb 3, 2021, at 11:39 AM, Borislav Petkov <bp@alien8.de> wrote:
> 
> On Sun, Jan 31, 2021 at 09:24:40AM -0800, Andy Lutomirski wrote:
>> The name no_context() has never been very clear.  It's only called for
>> faults from kernel mode, so rename it and change the no-longer-useful
>> user_mode(regs) check to a WARN_ON_ONCE.
>> 
>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>> arch/x86/mm/fault.c | 28 ++++++++++------------------
>> 1 file changed, 10 insertions(+), 18 deletions(-)
>> 
>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> index 177b612c7f33..04cc98ec2423 100644
>> --- a/arch/x86/mm/fault.c
>> +++ b/arch/x86/mm/fault.c
>> @@ -693,17 +693,10 @@ page_fault_oops(struct pt_regs *regs, unsigned long error_code,
>> }
>> 
>> static noinline void
>> -no_context(struct pt_regs *regs, unsigned long error_code,
>> -       unsigned long address, int signal, int si_code)
>> +kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
>> +             unsigned long address, int signal, int si_code)
> 
> Ew, I don't like functions with "or" in the name - they're probably not
> doing one thing only as they should.
> 
> Why not simply "handle_kernel_fault" ?

Hmm, I could maybe get behind handle_kernelmode_fault.  I’ll contemplate it.  I like the name of the function indicating that either it returns after fixing it or it doesn’t return.

I refuse to say “kernel” without qualification. In this $@!$ file, we have kernel _mode_, kernel _address_, and kernel _privilege_, and they are all different.

> 
> Also, all the callsites now do:
> 
>    if (!user_mode(regs)) {
>        kernelmode_fixup_or_oops
>        ...
> 
> I guess you can push the "user_mode" check inside that function for less
> hairy code at the callsites.

I feel like that would be more obfuscated — then the function would return without fixing anything for usermode faults, return after fixing it for kernel mode faults, or oops.

> 
>> {
>> -    if (user_mode(regs)) {
>> -        /*
>> -         * This is an implicit supervisor-mode access from user
>> -         * mode.  Bypass all the kernel-mode recovery code and just
>> -         * OOPS.
>> -         */
>> -        goto oops;
>> -    }
>> +    WARN_ON_ONCE(user_mode(regs));
> 
> I guess...
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 09/11] x86/fault: Rename no_context() to kernelmode_fixup_or_oops()

[Borislav Petkov]

On Wed, Feb 03, 2021 at 11:53:03AM -0800, Andy Lutomirski wrote:
> I feel like that would be more obfuscated — then the function would
> return without fixing anything for usermode faults, return after
> fixing it for kernel mode faults, or oops.

You practically pretty much have it already with the WARN_ON_ONCE. And
you can make the thing return 1 to denote it was in user_mode() and 0
otherwise. IINMSO, something like this:

---
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 08f5f74cf989..2b86d541b181 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -692,11 +692,12 @@ page_fault_oops(struct pt_regs *regs, unsigned long error_code,
 	oops_end(flags, regs, sig);
 }
 
-static noinline void
+static noinline int
 kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
 			 unsigned long address, int signal, int si_code)
 {
-	WARN_ON_ONCE(user_mode(regs));
+	if (WARN_ON_ONCE(user_mode(regs)))
+		return 1;
 
 	/* Are we prepared to handle this kernel fault? */
 	if (fixup_exception(regs, X86_TRAP_PF, error_code, address)) {
@@ -706,7 +707,7 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
 		 * task context.
 		 */
 		if (in_interrupt())
-			return;
+			return 0;
 
 		/*
 		 * Per the above we're !in_interrupt(), aka. task context.
@@ -726,7 +727,7 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
 		/*
 		 * Barring that, we can do the fixup and be happy.
 		 */
-		return;
+		return 0;
 	}
 
 	/*
@@ -734,9 +735,11 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
 	 * instruction.
 	 */
 	if (is_prefetch(regs, error_code, address))
-		return;
+		return 0;
 
 	page_fault_oops(regs, error_code, address);
+
+	return 0;
 }
 
 /*
@@ -781,10 +784,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 {
 	struct task_struct *tsk = current;
 
-	if (!user_mode(regs)) {
-		kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code);
+	if (!kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code))
 		return;
-	}
 
 	if (!(error_code & X86_PF_USER)) {
 		/* Implicit user access to kernel memory -- just oops */
@@ -913,10 +914,8 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 	  vm_fault_t fault)
 {
 	/* Kernel mode? Handle exceptions or die: */
-	if (!user_mode(regs)) {
-		kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR);
+	if (!kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR))
 		return;
-	}
 
 	/* User-space => ok to do another page fault: */
 	if (is_prefetch(regs, error_code, address))
@@ -1378,10 +1377,8 @@ void do_user_addr_fault(struct pt_regs *regs,
 		 * Quick path to respond to signals.  The core mm code
 		 * has unlocked the mm for us if we get here.
 		 */
-		if (!user_mode(regs))
-			kernelmode_fixup_or_oops(regs, error_code, address,
-						 SIGBUS, BUS_ADRERR);
-		return;
+		if (!kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR))
+			return;
 	}
 
 	/*
@@ -1399,18 +1396,15 @@ void do_user_addr_fault(struct pt_regs *regs,
 	if (likely(!(fault & VM_FAULT_ERROR)))
 		return;
 
-	if (fatal_signal_pending(current) && !user_mode(regs)) {
-		kernelmode_fixup_or_oops(regs, error_code, address, 0, 0);
-		return;
+	if (fatal_signal_pending(current)) {
+		if (!kernelmode_fixup_or_oops(regs, error_code, address, 0, 0))
+			return;
 	}
 
 	if (fault & VM_FAULT_OOM) {
 		/* Kernel mode? Handle exceptions or die: */
-		if (!user_mode(regs)) {
-			kernelmode_fixup_or_oops(regs, error_code, address,
-						 SIGSEGV, SEGV_MAPERR);
+		if (!kernelmode_fixup_or_oops(regs, error_code, address, SIGSEGV, SEGV_MAPERR))
 			return;
-		}
 
 		/*
 		 * We ran out of memory, call the OOM killer, and return the

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 09/11] x86/fault: Rename no_context() to kernelmode_fixup_or_oops()

[Andy Lutomirski]

On Wed, Feb 3, 2021 at 12:07 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Feb 03, 2021 at 11:53:03AM -0800, Andy Lutomirski wrote:
> > I feel like that would be more obfuscated — then the function would
> > return without fixing anything for usermode faults, return after
> > fixing it for kernel mode faults, or oops.
>
> You practically pretty much have it already with the WARN_ON_ONCE. And
> you can make the thing return 1 to denote it was in user_mode() and 0
> otherwise. IINMSO, something like this:

Hmm.  I'm not convinced this is really much better.  Maybe it is.  Let
me think about it.  I feel like it's somehow too close to the previous
tangle where too many functions did too many different things.

Re: [PATCH 09/11] x86/fault: Rename no_context() to kernelmode_fixup_or_oops()

[Borislav Petkov]

On Wed, Feb 03, 2021 at 12:14:24PM -0800, Andy Lutomirski wrote:
> Hmm.  I'm not convinced this is really much better.  Maybe it is.  Let
> me think about it.  I feel like it's somehow too close to the previous
> tangle where too many functions did too many different things.

I know what you mean.

Well, we can put it aside for now since you'll likely do more cleanup
here and it might become unnecessary after you're done. :-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 07/11] x86/fault: Split the OOPS code out from no_context()

[Andy Lutomirski]

On Wed, Feb 3, 2021 at 10:56 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Sun, Jan 31, 2021 at 09:24:38AM -0800, Andy Lutomirski wrote:
> > Not all callers of no_context() want to run exception fixups.
> > Separate the OOPS code out from the fixup code in no_context().
> >
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > ---
> >  arch/x86/mm/fault.c | 116 +++++++++++++++++++++++---------------------
> >  1 file changed, 62 insertions(+), 54 deletions(-)
> >
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index 1939e546beae..6f43d080e1e8 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -618,53 +618,20 @@ static void set_signal_archinfo(unsigned long address,
> >  }
> >
> >  static noinline void
> > -no_context(struct pt_regs *regs, unsigned long error_code,
> > -        unsigned long address, int signal, int si_code)
> > +page_fault_oops(struct pt_regs *regs, unsigned long error_code,
>
> Not sure about this name - it still tries to recover:
> efi_recover_from_page_fault().
>
> Judging by where it is called, maybe no_context_tail() or
> no_context_oops() or no_context_finish_me_already()...
>
> Yah, I haz no better idea. :-\
>
> ...
>
> > @@ -739,6 +692,61 @@ no_context(struct pt_regs *regs, unsigned long error_code,
> >       oops_end(flags, regs, sig);
> >  }
> >
> > +static noinline void
> > +no_context(struct pt_regs *regs, unsigned long error_code,
> > +        unsigned long address, int signal, int si_code)
> > +{
> > +     if (user_mode(regs)) {
> > +             /*
> > +              * This is an implicit supervisor-mode access from user
> > +              * mode.  Bypass all the kernel-mode recovery code and just
> > +              * OOPS.
> > +              */
> > +             goto oops;
>
> Just do
>
>                 return page_fault_oops(...);
>
> here and get rid of the label.

I'm going to skip this one, because that code is deleted later in the
series, and fixing it here just adds more churn.