Skip to contentSkip to conversation

user_mode_vm removal and associated cleanups

PATCH In review Andy Lutomirski 7 participants 31 messages Mar 19, 2015 – Mar 31, 2015

Subsystems (7)
x86 perf events Char and misc drivers x86 MM +3 more subsystems
lore
Share & tools
lore

Andy Lutomirski

author maintainer
#0 lore raw

cover user_mode_vm removal and associated cleanups

Hi all-

The user_mode vs user_mode_vm distinction scares me. Let's fix it. This series adds user_mode_ignore_vm86, makes user_mode reliable, and removes user_mode_vm. It also tidies up a couple warts I found along the way.

This survives basic testing, but I haven't tried that hard to test it.

Thoughts?

Ingo, this may conflict a bit with the do_debug and do_bounds fixes.

Andy Lutomirski (9):
x86, fault: Use TASK_SIZE_MAX in is_prefetch
x86, perf: Fix incorrect TIF_IA32 check in code_segment_base
x86: Add user_mode_ignore_vm86
x86, perf: Explicitly optimize vm86 handling in code_segment_base x86, traps: Use user_mode_ignore_vm86 where appropriate
x86: Make user_mode work correctly if regs came from vm86 mode x86, treewide: s/user_mode_vm/user_mode/g
x86: Remove user_mode_vm
x86, traps: Replace some open-coded vm86 checks with v8086_mode

 arch/x86/include/asm/ptrace.h    | 33 +++++++++++++++++++++------------
 arch/x86/kernel/alternative.c    |  2 +-
 arch/x86/kernel/cpu/perf_event.c | 20 ++++++++++----------
 arch/x86/kernel/crash.c          |  2 +-
 arch/x86/kernel/dumpstack.c      |  4 ++--
 arch/x86/kernel/dumpstack_32.c   |  4 ++--
 arch/x86/kernel/i387.c           |  2 +-
 arch/x86/kernel/irq_32.c         |  2 +-
 arch/x86/kernel/irq_64.c         |  2 +-
 arch/x86/kernel/kgdb.c           |  4 ++--
 arch/x86/kernel/kprobes/core.c   |  4 ++--
 arch/x86/kernel/process_32.c     |  2 +-
 arch/x86/kernel/ptrace.c         |  2 +-
 arch/x86/kernel/time.c           |  2 +-
 arch/x86/kernel/traps.c          | 29 +++++++++++++----------------
 arch/x86/kernel/uprobes.c        |  2 +-
 arch/x86/mm/fault.c              |  8 ++++----
 arch/x86/oprofile/backtrace.c    |  2 +-
 drivers/misc/sgi-xp/xpc_main.c   |  2 +-
 19 files changed, 67 insertions(+), 61 deletions(-)
Signature · 1 line
2.3.0
author maintainer
#3 lore raw

PATCH 2/9 x86, perf: Fix incorrect TIF_IA32 check in code_segment_base

We're want to check whether user code is in 32-bit mode, not whether the task is nominally 32-bit.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/kernel/cpu/perf_event.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
arch/x86/kernel/cpu/perf_event.c +3 -4
index b71a7f86d68a..979963bb3977 100644
@@ -2161,10 +2161,9 @@ static unsigned long code_segment_base(struct pt_regs *regs)
 	if (user_mode(regs) && regs->cs != __USER_CS)
 		return get_segment_base(regs->cs);
 #else
-	if (test_thread_flag(TIF_IA32)) {
-		if (user_mode(regs) && regs->cs != __USER32_CS)
-			return get_segment_base(regs->cs);
-	}
+	if (user_mode(regs) && !user_64bit_mode(regs) &&
+	    regs->cs != __USER32_CS)
+		return get_segment_base(regs->cs);
 #endif
 	return 0;
 }
Signature · 1 line
2.3.0
author maintainer
#5 lore raw

PATCH 3/9 x86: Add user_mode_ignore_vm86

In reply to Andy Lutomirski

user_mode is dangerous and user_mode_vm has a confusing name. Add user_mode_ignore_vm86 (equivalent to current user_mode). We'll change the small number of legitimate users of user_mode to user_mode_ignore_vm86.

Inspired by grsec, although this works rather differently.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/ptrace.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
arch/x86/include/asm/ptrace.h +17 -0
index 74bb2e0f3030..a60c59e977cc 100644
@@ -121,6 +121,23 @@ static inline int user_mode_vm(struct pt_regs *regs)
 #endif
 }
 
+/*
+ * This is the fastest way to check whether regs come from user space.
+ * It is unsafe if regs might come from vm86 mode, though -- in vm86
+ * mode, all bits of CS and SS are completely under the user's control.
+ * The CPU considers vm86 mode to be CPL 3 regardless of CS and SS.
+ *
+ * Do NOT use this function unless you have already ruled out the
+ * possibility that regs came from vm86 mode.
+ *
+ * We check for RPL != 0 instead of RPL == 3 because we don't use rings
+ * 1 or 2 and this is more efficient.
+ */
+static inline int user_mode_ignore_vm86(struct pt_regs *regs)
+{
+	return (regs->cs & SEGMENT_RPL_MASK) != 0;
+}
+
 static inline int v8086_mode(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_32
Signature · 1 line
2.3.0
author maintainer
#28 lore raw

PATCH 9/9 x86, traps: Replace some open-coded vm86 checks with v8086_mode

In reply to Andy Lutomirski

This allows us to remove some unnecessary ifdefs. There should be no change to the generated code.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/kernel/traps.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
arch/x86/kernel/traps.c +4 -7
index d4e265952102..c8eb469a94a4 100644
@@ -194,8 +194,7 @@ static nokprobe_inline int
 do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		  struct pt_regs *regs,	long error_code)
 {
-#ifdef CONFIG_X86_32
-	if (regs->flags & X86_VM_MASK) {
+	if (v8086_mode(regs)) {
 		/*
 		 * Traps 0, 1, 3, 4, and 5 should be forwarded to vm86.
 		 * On nmi (interrupt 2), do_trap should not be called.

@@ -207,7 +206,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		}
 		return -1;
 	}
-#endif
+
 	if (!user_mode_ignore_vm86(regs)) {
 		if (!fixup_exception(regs)) {
 			tsk->thread.error_code = error_code;

@@ -462,13 +461,11 @@ do_general_protection(struct pt_regs *regs, long error_code)
 	prev_state = exception_enter();
 	conditional_sti(regs);
 
-#ifdef CONFIG_X86_32
-	if (regs->flags & X86_VM_MASK) {
+	if (v8086_mode(regs)) {
 		local_irq_enable();
 		handle_vm86_fault((struct kernel_vm86_regs *) regs, error_code);
 		goto exit;
 	}
-#endif
 
 	tsk = current;
 	if (!user_mode_ignore_vm86(regs)) {

@@ -673,7 +670,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	/* It's safe to allow irq's after DR6 has been saved */
 	preempt_conditional_sti(regs);
 
-	if (regs->flags & X86_VM_MASK) {
+	if (v8086_mode(regs)) {
 		handle_vm86_trap((struct kernel_vm86_regs *) regs, error_code,
 					X86_TRAP_DB);
 		preempt_conditional_cli(regs);
Signature · 1 line
2.3.0
author maintainer
#26 lore raw

PATCH 8/9 x86: Remove user_mode_vm

In reply to Andy Lutomirski

It has no callers any more.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/ptrace.h | 5 -----
 1 file changed, 5 deletions(-)
arch/x86/include/asm/ptrace.h +0 -5
index 6483525bb559..953675c247a3 100644
@@ -113,11 +113,6 @@ static inline int user_mode(struct pt_regs *regs)
 #endif
 }
 
-static inline int user_mode_vm(struct pt_regs *regs)
-{
-	return user_mode(regs);
-}
-
 /*
  * This is the fastest way to check whether regs come from user space.
  * It is unsafe if regs might come from vm86 mode, though -- in vm86
Signature · 1 line
2.3.0
author maintainer
#24 lore raw

PATCH 7/9 x86, treewide: s/user_mode_vm/user_mode/g

In reply to Andy Lutomirski

user_mode_vm and user_mode are now the same. Change all callers of user_mode_vm to user_mode.

The next patch will remove the definition of user_mode_vm.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/kernel/alternative.c  |  2 +-
 arch/x86/kernel/crash.c        |  2 +-
 arch/x86/kernel/dumpstack.c    |  4 ++--
 arch/x86/kernel/dumpstack_32.c |  4 ++--
 arch/x86/kernel/i387.c         |  2 +-
 arch/x86/kernel/irq_32.c       |  2 +-
 arch/x86/kernel/irq_64.c       |  2 +-
 arch/x86/kernel/kgdb.c         |  4 ++--
 arch/x86/kernel/kprobes/core.c |  4 ++--
 arch/x86/kernel/process_32.c   |  2 +-
 arch/x86/kernel/ptrace.c       |  2 +-
 arch/x86/kernel/time.c         |  2 +-
 arch/x86/kernel/traps.c        | 12 ++++++------
 arch/x86/kernel/uprobes.c      |  2 +-
 arch/x86/mm/fault.c            |  6 +++---
 arch/x86/oprofile/backtrace.c  |  2 +-
 drivers/misc/sgi-xp/xpc_main.c |  2 +-
 17 files changed, 28 insertions(+), 28 deletions(-)
arch/x86/kernel/alternative.c +1 -1
index af397cc98d05..5c993c94255e 100644
@@ -715,7 +715,7 @@ int poke_int3_handler(struct pt_regs *regs)
 	if (likely(!bp_patching_in_progress))
 		return 0;
 
-	if (user_mode_vm(regs) || regs->ip != (unsigned long)bp_int3_addr)
+	if (user_mode(regs) || regs->ip != (unsigned long)bp_int3_addr)
 		return 0;
 
 	/* set up the specified breakpoint handler */
arch/x86/kernel/crash.c +1 -1
index aceb2f90c716..c76d3e37c6e1 100644
@@ -105,7 +105,7 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
 #ifdef CONFIG_X86_32
 	struct pt_regs fixed_regs;
 
-	if (!user_mode_vm(regs)) {
+	if (!user_mode(regs)) {
 		crash_fixup_ss_esp(&fixed_regs, regs);
 		regs = &fixed_regs;
 	}
arch/x86/kernel/dumpstack.c +2 -2
index cf3df1d8d039..ab3b65639a3e 100644
@@ -278,7 +278,7 @@ int __die(const char *str, struct pt_regs *regs, long err)
 	print_modules();
 	show_regs(regs);
 #ifdef CONFIG_X86_32
-	if (user_mode_vm(regs)) {
+	if (user_mode(regs)) {
 		sp = regs->sp;
 		ss = regs->ss & 0xffff;
 	} else {

@@ -307,7 +307,7 @@ void die(const char *str, struct pt_regs *regs, long err)
 	unsigned long flags = oops_begin();
 	int sig = SIGSEGV;
 
-	if (!user_mode_vm(regs))
+	if (!user_mode(regs))
 		report_bug(regs->ip, regs);
 
 	if (__die(str, regs, err))
arch/x86/kernel/dumpstack_32.c +2 -2
index 5abd4cd4230c..39891ff50d03 100644
@@ -123,13 +123,13 @@ void show_regs(struct pt_regs *regs)
 	int i;
 
 	show_regs_print_info(KERN_EMERG);
-	__show_regs(regs, !user_mode_vm(regs));
+	__show_regs(regs, !user_mode(regs));
 
 	/*
 	 * When in-kernel, we also print out the stack and code at the
 	 * time of the fault..
 	 */
-	if (!user_mode_vm(regs)) {
+	if (!user_mode(regs)) {
 		unsigned int code_prologue = code_bytes * 43 / 64;
 		unsigned int code_len = code_bytes;
 		unsigned char c;
arch/x86/kernel/i387.c +1 -1
index d5651fce0b71..29c740deafec 100644
@@ -68,7 +68,7 @@ static inline bool interrupted_kernel_fpu_idle(void)
 static inline bool interrupted_user_mode(void)
 {
 	struct pt_regs *regs = get_irq_regs();
-	return regs && user_mode_vm(regs);
+	return regs && user_mode(regs);
 }
 
 /*
arch/x86/kernel/irq_32.c +1 -1
index 28d28f5eb8f4..f9fd86a7fcc7 100644
@@ -165,7 +165,7 @@ bool handle_irq(unsigned irq, struct pt_regs *regs)
 	if (unlikely(!desc))
 		return false;
 
-	if (user_mode_vm(regs) || !execute_on_irq_stack(overflow, desc, irq)) {
+	if (user_mode(regs) || !execute_on_irq_stack(overflow, desc, irq)) {
 		if (unlikely(overflow))
 			print_stack_overflow();
 		desc->handle_irq(irq, desc);
arch/x86/kernel/irq_64.c +1 -1
index e4b503d5558c..394e643d7830 100644
@@ -44,7 +44,7 @@ static inline void stack_overflow_check(struct pt_regs *regs)
 	u64 estack_top, estack_bottom;
 	u64 curbase = (u64)task_stack_page(current);
 
-	if (user_mode_vm(regs))
+	if (user_mode(regs))
 		return;
 
 	if (regs->sp >= curbase + sizeof(struct thread_info) +
arch/x86/kernel/kgdb.c +2 -2
index 7ec1d5f8d283..7fe3a9d377ea 100644
@@ -126,11 +126,11 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs)
 #ifdef CONFIG_X86_32
 	switch (regno) {
 	case GDB_SS:
-		if (!user_mode_vm(regs))
+		if (!user_mode(regs))
 			*(unsigned long *)mem = __KERNEL_DS;
 		break;
 	case GDB_SP:
-		if (!user_mode_vm(regs))
+		if (!user_mode(regs))
 			*(unsigned long *)mem = kernel_stack_pointer(regs);
 		break;
 	case GDB_GS:
arch/x86/kernel/kprobes/core.c +2 -2
index 4e3d5a9621fe..24d079604fd5 100644
@@ -602,7 +602,7 @@ int kprobe_int3_handler(struct pt_regs *regs)
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
 
-	if (user_mode_vm(regs))
+	if (user_mode(regs))
 		return 0;
 
 	addr = (kprobe_opcode_t *)(regs->ip - sizeof(kprobe_opcode_t));

@@ -1007,7 +1007,7 @@ int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val,
 	struct die_args *args = data;
 	int ret = NOTIFY_DONE;
 
-	if (args->regs && user_mode_vm(args->regs))
+	if (args->regs && user_mode(args->regs))
 		return ret;
 
 	if (val == DIE_GPF) {
arch/x86/kernel/process_32.c +1 -1
index 1b9963faf4eb..0973aada656f 100644
@@ -73,7 +73,7 @@ void __show_regs(struct pt_regs *regs, int all)
 	unsigned long sp;
 	unsigned short ss, gs;
 
-	if (user_mode_vm(regs)) {
+	if (user_mode(regs)) {
 		sp = regs->sp;
 		ss = regs->ss & 0xffff;
 		gs = get_user_gs(regs);
arch/x86/kernel/ptrace.c +1 -1
index 1e125817cf9f..a7bc79480719 100644
@@ -1415,7 +1415,7 @@ static void fill_sigtrap_info(struct task_struct *tsk,
 	memset(info, 0, sizeof(*info));
 	info->si_signo = SIGTRAP;
 	info->si_code = si_code;
-	info->si_addr = user_mode_vm(regs) ? (void __user *)regs->ip : NULL;
+	info->si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL;
 }
 
 void user_single_step_siginfo(struct task_struct *tsk,
arch/x86/kernel/time.c +1 -1
index 25adc0e16eaa..d39c09119db6 100644
@@ -30,7 +30,7 @@ unsigned long profile_pc(struct pt_regs *regs)
 {
 	unsigned long pc = instruction_pointer(regs);
 
-	if (!user_mode_vm(regs) && in_lock_functions(pc)) {
+	if (!user_mode(regs) && in_lock_functions(pc)) {
 #ifdef CONFIG_FRAME_POINTER
 		return *(unsigned long *)(regs->bp + sizeof(long));
 #else
arch/x86/kernel/traps.c +6 -6
index 376fc1562bd1..d4e265952102 100644
@@ -112,7 +112,7 @@ enum ctx_state ist_enter(struct pt_regs *regs)
 {
 	enum ctx_state prev_state;
 
-	if (user_mode_vm(regs)) {
+	if (user_mode(regs)) {
 		/* Other than that, we're just an exception. */
 		prev_state = exception_enter();
 	} else {

@@ -146,7 +146,7 @@ void ist_exit(struct pt_regs *regs, enum ctx_state prev_state)
 	/* Must be before exception_exit. */
 	preempt_count_sub(HARDIRQ_OFFSET);
 
-	if (user_mode_vm(regs))
+	if (user_mode(regs))
 		return exception_exit(prev_state);
 	else
 		rcu_nmi_exit();

@@ -158,7 +158,7 @@ void ist_exit(struct pt_regs *regs, enum ctx_state prev_state)
  *
  * IST exception handlers normally cannot schedule.  As a special
  * exception, if the exception interrupted userspace code (i.e.
- * user_mode_vm(regs) would return true) and the exception was not
+ * user_mode(regs) would return true) and the exception was not
  * a double fault, it can be safe to schedule.  ist_begin_non_atomic()
  * begins a non-atomic section within an ist_enter()/ist_exit() region.
  * Callers are responsible for enabling interrupts themselves inside

@@ -167,7 +167,7 @@ void ist_exit(struct pt_regs *regs, enum ctx_state prev_state)
  */
 void ist_begin_non_atomic(struct pt_regs *regs)
 {
-	BUG_ON(!user_mode_vm(regs));
+	BUG_ON(!user_mode(regs));
 
 	/*
 	 * Sanity check: we need to be on the normal thread stack.  This

@@ -587,7 +587,7 @@ struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
 	/* Copy the remainder of the stack from the current stack. */
 	memmove(new_stack, s, offsetof(struct bad_iret_stack, regs.ip));
 
-	BUG_ON(!user_mode_vm(&new_stack->regs));
+	BUG_ON(!user_mode(&new_stack->regs));
 	return new_stack;
 }
 NOKPROBE_SYMBOL(fixup_bad_iret);

@@ -721,7 +721,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 		return;
 	conditional_sti(regs);
 
-	if (!user_mode_vm(regs))
+	if (!user_mode(regs))
 	{
 		if (!fixup_exception(regs)) {
 			task->thread.error_code = error_code;
arch/x86/kernel/uprobes.c +1 -1
index 81f8adb0679e..0b81ad67da07 100644
@@ -912,7 +912,7 @@ int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val,
 	int ret = NOTIFY_DONE;
 
 	/* We are only interested in userspace traps */
-	if (regs && !user_mode_vm(regs))
+	if (regs && !user_mode(regs))
 		return NOTIFY_DONE;
 
 	switch (val) {
arch/x86/mm/fault.c +3 -3
index ae340d3761ca..181c53bac3a7 100644
@@ -59,7 +59,7 @@ static nokprobe_inline int kprobes_fault(struct pt_regs *regs)
 	int ret = 0;
 
 	/* kprobe_running() needs smp_processor_id() */
-	if (kprobes_built_in() && !user_mode_vm(regs)) {
+	if (kprobes_built_in() && !user_mode(regs)) {
 		preempt_disable();
 		if (kprobe_running() && kprobe_fault_handler(regs, 14))
 			ret = 1;

@@ -1035,7 +1035,7 @@ static inline bool smap_violation(int error_code, struct pt_regs *regs)
 	if (error_code & PF_USER)
 		return false;
 
-	if (!user_mode_vm(regs) && (regs->flags & X86_EFLAGS_AC))
+	if (!user_mode(regs) && (regs->flags & X86_EFLAGS_AC))
 		return false;
 
 	return true;

@@ -1140,7 +1140,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
 	 * User-mode registers count as a user access even for any
 	 * potential system fault or CPU buglet:
 	 */
-	if (user_mode_vm(regs)) {
+	if (user_mode(regs)) {
 		local_irq_enable();
 		error_code |= PF_USER;
 		flags |= FAULT_FLAG_USER;
arch/x86/oprofile/backtrace.c +1 -1
index 5d04be5efb64..4e664bdb535a 100644
@@ -111,7 +111,7 @@ x86_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
 	struct stack_frame *head = (struct stack_frame *)frame_pointer(regs);
 
-	if (!user_mode_vm(regs)) {
+	if (!user_mode(regs)) {
 		unsigned long stack = kernel_stack_pointer(regs);
 		if (depth)
 			dump_trace(NULL, regs, (unsigned long *)stack, 0,
drivers/misc/sgi-xp/xpc_main.c +1 -1
index 82dc5748f873..7f327121e6d7 100644
@@ -1210,7 +1210,7 @@ xpc_system_die(struct notifier_block *nb, unsigned long event, void *_die_args)
 
 		if (((die_args->trapnr == X86_TRAP_MF) ||
 		     (die_args->trapnr == X86_TRAP_XF)) &&
-		    !user_mode_vm(die_args->regs))
+		    !user_mode(die_args->regs))
 			xpc_die_deactivate();
 
 		break;
Signature · 1 line
2.3.0
author maintainer
#22 lore raw

PATCH 6/9 x86: Make user_mode work correctly if regs came from vm86 mode

In reply to Andy Lutomirski

user_mode is now identical to user_mode_vm. Subsequent patches will change all callers of user_mode_vm to user_mode and then delete user_mode_vm.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/ptrace.h | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
arch/x86/include/asm/ptrace.h +7 -10
index a60c59e977cc..6483525bb559 100644
@@ -96,11 +96,13 @@ static inline unsigned long regs_return_value(struct pt_regs *regs)
 }
 
 /*
- * user_mode_vm(regs) determines whether a register set came from user mode.
- * This is true if V8086 mode was enabled OR if the register set was from
- * protected mode with RPL-3 CS value.  This tricky test checks that with
- * one comparison.  Many places in the kernel can bypass this full check
- * if they have already ruled out V8086 mode, so user_mode(regs) can be used.
+ * user_mode(regs) determines whether a register set came from user
+ * mode.  On x86_32, this is true if V8086 mode was enabled OR if the
+ * register set was from protected mode with RPL-3 CS value.  This
+ * tricky test checks that with one comparison.
+ *
+ * On x86_64, vm86 mode is mercifully nonexistent, and we don't need
+ * the extra check.
  */
 static inline int user_mode(struct pt_regs *regs)
 {

@@ -113,12 +115,7 @@ static inline int user_mode(struct pt_regs *regs)
 
 static inline int user_mode_vm(struct pt_regs *regs)
 {
-#ifdef CONFIG_X86_32
-	return ((regs->cs & SEGMENT_RPL_MASK) | (regs->flags & X86_VM_MASK)) >=
-		USER_RPL;
-#else
 	return user_mode(regs);
-#endif
 }
 
 /*
Signature · 1 line
2.3.0
author maintainer
#20 lore raw

PATCH 5/9 x86, traps: Use user_mode_ignore_vm86 where appropriate

In reply to Andy Lutomirski

A few of the user_mode checks in traps.c are immediately after explicit checks for vm86 mode. Change them to user_mode_ignore_vm86.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/kernel/traps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/traps.c +3 -3
index 081252c44cde..376fc1562bd1 100644
@@ -208,7 +208,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		return -1;
 	}
 #endif
-	if (!user_mode(regs)) {
+	if (!user_mode_ignore_vm86(regs)) {
 		if (!fixup_exception(regs)) {
 			tsk->thread.error_code = error_code;
 			tsk->thread.trap_nr = trapnr;

@@ -471,7 +471,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
 #endif
 
 	tsk = current;
-	if (!user_mode(regs)) {
+	if (!user_mode_ignore_vm86(regs)) {
 		if (fixup_exception(regs))
 			goto exit;
 

@@ -688,7 +688,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	 * We already checked v86 mode above, so we can check for kernel mode
 	 * by just checking the CPL of CS.
 	 */
-	if ((dr6 & DR_STEP) && !user_mode(regs)) {
+	if ((dr6 & DR_STEP) && !user_mode_ignore_vm86(regs)) {
 		tsk->thread.debugreg6 &= ~DR_STEP;
 		set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
 		regs->flags &= ~X86_EFLAGS_TF;
Signature · 1 line
2.3.0
author maintainer
#18 lore raw

PATCH 4/9 x86, perf: Explicitly optimize vm86 handling in code_segment_base

In reply to Andy Lutomirski

There's no point in checking the VM bit on 64-bit, and, since we're explicitly checking it, we can use user_mode_ignore_vm86 after the check.

While we're at it, rearrange the ifdef slightly to make the code flow a bit clearer.

Cc Peter Zijlstra <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/kernel/cpu/perf_event.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
arch/x86/kernel/cpu/perf_event.c +7 -6
index 979963bb3977..56f7e60ad732 100644
@@ -2147,18 +2147,19 @@ perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs)
 static unsigned long code_segment_base(struct pt_regs *regs)
 {
 	/*
+	 * For IA32 we look at the GDT/LDT segment base to convert the
+	 * effective IP to a linear address.
+	 */
+
+#ifdef CONFIG_X86_32
+	/*
 	 * If we are in VM86 mode, add the segment offset to convert to a
 	 * linear address.
 	 */
 	if (regs->flags & X86_VM_MASK)
 		return 0x10 * regs->cs;
 
-	/*
-	 * For IA32 we look at the GDT/LDT segment base to convert the
-	 * effective IP to a linear address.
-	 */
-#ifdef CONFIG_X86_32
-	if (user_mode(regs) && regs->cs != __USER_CS)
+	if (user_mode_ignore_vm86(regs) && regs->cs != __USER_CS)
 		return get_segment_base(regs->cs);
 #else
 	if (user_mode(regs) && !user_64bit_mode(regs) &&
Signature · 1 line
2.3.0
author maintainer
#1 lore raw

PATCH 1/9 x86, fault: Use TASK_SIZE_MAX in is_prefetch

In reply to Andy Lutomirski

This is slightly shorter and slightly faster. It's also more
correct: the split between user and kernel addresses is TASK_SIZE_MAX regardless of ti->flags.

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/mm/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/mm/fault.c +1 -1
index ede025fb46f1..ae340d3761ca 100644
@@ -148,7 +148,7 @@ is_prefetch(struct pt_regs *regs, unsigned long error_code, unsigned long addr)
 	instr = (void *)convert_ip_to_linear(current, regs);
 	max_instr = instr + 15;
 
-	if (user_mode(regs) && instr >= (unsigned char *)TASK_SIZE)
+	if (user_mode(regs) && instr >= (unsigned char *)TASK_SIZE_MAX)
 		return 0;
 
 	while (instr < max_instr) {
Signature · 1 line
2.3.0
#30 lore raw
In reply to Andy Lutomirski
Quoted 7 lines · Andy Lutomirski

Hi all-

The user_mode vs user_mode_vm distinction scares me. Let's fix it. This series adds user_mode_ignore_vm86, makes user_mode reliable, and removes user_mode_vm. It also tidies up a couple warts I found along the way.

Quoted 5 lines

This survives basic testing, but I haven't tried that hard to test it.

Thoughts?

Ingo, this may conflict a bit with the do_debug and do_bounds fixes.

I like it, in fact I'd suggest we remove user_mode_ignore_vm86() altogether, as it's such a marginal optimization, it only affects x86-32 kernels, and because we keep getting this wrong.

Keep a single, simple user_mode() definition.

Thanks,

Ingo

merge bot
#4 lore raw

[tip:x86/asm] x86/asm/entry, perf: Fix incorrect TIF_IA32 check in code_segment_base()

In reply to Andy Lutomirski
x86/asm/entry, perf: Fix incorrect TIF_IA32 check in code_segment_base()
Commit-ID:  c56716af8d27ca8dd6e45445ae1c0a05fd9753a6
Gitweb:     http://git.kernel.org/tip/c56716af8d27ca8dd6e45445ae1c0a05fd9753a6
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:28 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 10:08:21 +0100

x86/asm/entry, perf: Fix incorrect TIF_IA32 check in code_segment_base()

We want to check whether user code is in 32-bit mode, not whether the task is nominally 32-bit.

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/33e5107085ce347a8303560302b15c2cadd62c4c.1426728647.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/cpu/perf_event.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
arch/x86/kernel/cpu/perf_event.c +3 -4
index b71a7f8..979963b 100644
@@ -2161,10 +2161,9 @@ static unsigned long code_segment_base(struct pt_regs *regs)
 	if (user_mode(regs) && regs->cs != __USER_CS)
 		return get_segment_base(regs->cs);
 #else
-	if (test_thread_flag(TIF_IA32)) {
-		if (user_mode(regs) && regs->cs != __USER32_CS)
-			return get_segment_base(regs->cs);
-	}
+	if (user_mode(regs) && !user_64bit_mode(regs) &&
+	    regs->cs != __USER32_CS)
+		return get_segment_base(regs->cs);
 #endif
 	return 0;
 }
merge bot
#2 lore raw

[tip:x86/asm] x86/mm/fault: Use TASK_SIZE_MAX in is_prefetch()

In reply to Andy Lutomirski
x86/mm/fault: Use TASK_SIZE_MAX in is_prefetch()
Commit-ID:  d31bf07f71a5568b48c5ed448e4299050469f615
Gitweb:     http://git.kernel.org/tip/d31bf07f71a5568b48c5ed448e4299050469f615
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:27 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 10:08:20 +0100

x86/mm/fault: Use TASK_SIZE_MAX in is_prefetch()

This is slightly shorter and slightly faster. It's also more
correct: the split between user and kernel addresses is TASK_SIZE_MAX, regardless of ti->flags.

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/09156b63bad90a327827003c9e53faa82ef4c56e.1426728647.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/mm/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/mm/fault.c +1 -1
index ede025f..ae340d3 100644
@@ -148,7 +148,7 @@ is_prefetch(struct pt_regs *regs, unsigned long error_code, unsigned long addr)
 	instr = (void *)convert_ip_to_linear(current, regs);
 	max_instr = instr + 15;
 
-	if (user_mode(regs) && instr >= (unsigned char *)TASK_SIZE)
+	if (user_mode(regs) && instr >= (unsigned char *)TASK_SIZE_MAX)
 		return 0;
 
 	while (instr < max_instr) {
merge bot
#6 lore raw

[tip:x86/asm] x86/asm/entry: Add user_mode_ignore_vm86()

In reply to Andy Lutomirski
x86/asm/entry: Add user_mode_ignore_vm86()
Commit-ID:  a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Gitweb:     http://git.kernel.org/tip/a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:29 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:13:36 +0100

x86/asm/entry: Add user_mode_ignore_vm86()

user_mode() is dangerous and user_mode_vm() has a confusing name.

Add user_mode_ignore_vm86() (equivalent to current user_mode()). We'll change the small number of legitimate users of user_mode() to user_mode_ignore_vm86().

Inspired by grsec, although this works rather differently.

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/202c56ca63823c338af8e2e54948dbe222da6343.1426728647.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/include/asm/ptrace.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
arch/x86/include/asm/ptrace.h +17 -0
index 83b874d..4a040f0 100644
@@ -121,6 +121,23 @@ static inline int user_mode_vm(struct pt_regs *regs)
 #endif
 }
 
+/*
+ * This is the fastest way to check whether regs come from user space.
+ * It is unsafe if regs might come from vm86 mode, though -- in vm86
+ * mode, all bits of CS and SS are completely under the user's control.
+ * The CPU considers vm86 mode to be CPL 3 regardless of CS and SS.
+ *
+ * Do NOT use this function unless you have already ruled out the
+ * possibility that regs came from vm86 mode.
+ *
+ * We check for RPL != 0 instead of RPL == 3 because we don't use rings
+ * 1 or 2 and this is more efficient.
+ */
+static inline int user_mode_ignore_vm86(struct pt_regs *regs)
+{
+	return (regs->cs & SEGMENT_RPL_MASK) != 0;
+}
+
 static inline int v8086_mode(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_32
merge bot
#19 lore raw

[tip:x86/asm] x86/asm/entry, perf: Explicitly optimize vm86 handling in code_segment_base()

In reply to Andy Lutomirski
x86/asm/entry, perf: Explicitly optimize vm86 handling in code_segment_base()
Commit-ID:  383f3af3f88aadafe1fcf1948987ad538683fb8c
Gitweb:     http://git.kernel.org/tip/383f3af3f88aadafe1fcf1948987ad538683fb8c
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:30 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:13:41 +0100

x86/asm/entry, perf: Explicitly optimize vm86 handling in code_segment_base()

There's no point in checking the VM bit on 64-bit, and, since we're explicitly checking it, we can use user_mode_ignore_vm86() after the check.

While we're at it, rearrange the #ifdef slightly to make the code flow a bit clearer.

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/dc1457a734feccd03a19bb3538a7648582f57cdd.1426728647.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/cpu/perf_event.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
arch/x86/kernel/cpu/perf_event.c +7 -6
index 979963b..56f7e60 100644
@@ -2147,18 +2147,19 @@ perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs)
 static unsigned long code_segment_base(struct pt_regs *regs)
 {
 	/*
+	 * For IA32 we look at the GDT/LDT segment base to convert the
+	 * effective IP to a linear address.
+	 */
+
+#ifdef CONFIG_X86_32
+	/*
 	 * If we are in VM86 mode, add the segment offset to convert to a
 	 * linear address.
 	 */
 	if (regs->flags & X86_VM_MASK)
 		return 0x10 * regs->cs;
 
-	/*
-	 * For IA32 we look at the GDT/LDT segment base to convert the
-	 * effective IP to a linear address.
-	 */
-#ifdef CONFIG_X86_32
-	if (user_mode(regs) && regs->cs != __USER_CS)
+	if (user_mode_ignore_vm86(regs) && regs->cs != __USER_CS)
 		return get_segment_base(regs->cs);
 #else
 	if (user_mode(regs) && !user_64bit_mode(regs) &&
merge bot
#21 lore raw

[tip:x86/asm] x86/asm/entry: Use user_mode_ignore_vm86() where appropriate

In reply to Andy Lutomirski
x86/asm/entry: Use user_mode_ignore_vm86() where appropriate
Commit-ID:  ae60f0710ae6b33092267ef8ac853c498f6d3e5d
Gitweb:     http://git.kernel.org/tip/ae60f0710ae6b33092267ef8ac853c498f6d3e5d
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:31 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:13:46 +0100

x86/asm/entry: Use user_mode_ignore_vm86() where appropriate

A few of the user_mode() checks in traps.c are immediately after explicit checks for vm86 mode. Change them to user_mode_ignore_vm86().

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/0b324d5b75c3402be07f8d3c6245ed7f4995029e.1426728647.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/traps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/traps.c +3 -3
index 2773411..1136961 100644
@@ -208,7 +208,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		return -1;
 	}
 #endif
-	if (!user_mode(regs)) {
+	if (!user_mode_ignore_vm86(regs)) {
 		if (!fixup_exception(regs)) {
 			tsk->thread.error_code = error_code;
 			tsk->thread.trap_nr = trapnr;

@@ -471,7 +471,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
 #endif
 
 	tsk = current;
-	if (!user_mode(regs)) {
+	if (!user_mode_ignore_vm86(regs)) {
 		if (fixup_exception(regs))
 			goto exit;
 

@@ -688,7 +688,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	 * We already checked v86 mode above, so we can check for kernel mode
 	 * by just checking the CPL of CS.
 	 */
-	if ((dr6 & DR_STEP) && !user_mode(regs)) {
+	if ((dr6 & DR_STEP) && !user_mode_ignore_vm86(regs)) {
 		tsk->thread.debugreg6 &= ~DR_STEP;
 		set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
 		regs->flags &= ~X86_EFLAGS_TF;
merge bot
#23 lore raw

[tip:x86/asm] x86/asm/entry: Make user_mode() work correctly if regs came from VM86 mode

In reply to Andy Lutomirski
x86/asm/entry: Make user_mode() work correctly if regs came from VM86 mode
Commit-ID:  efa704510342b81ae58d7b8a0c7f676a4289b603
Gitweb:     http://git.kernel.org/tip/efa704510342b81ae58d7b8a0c7f676a4289b603
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:32 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:13:51 +0100

x86/asm/entry: Make user_mode() work correctly if regs came from VM86 mode

user_mode() is now identical to user_mode_vm(). Subsequent patches will change all callers of user_mode_vm() to user_mode() and then delete user_mode_vm().

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/0dd03eacb5f0a2b5ba0240de25347a31b493c289.1426728647.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/include/asm/ptrace.h | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
arch/x86/include/asm/ptrace.h +7 -10
index 4a040f0..70c439f 100644
@@ -96,11 +96,13 @@ static inline unsigned long regs_return_value(struct pt_regs *regs)
 }
 
 /*
- * user_mode_vm(regs) determines whether a register set came from user mode.
- * This is true if V8086 mode was enabled OR if the register set was from
- * protected mode with RPL-3 CS value.  This tricky test checks that with
- * one comparison.  Many places in the kernel can bypass this full check
- * if they have already ruled out V8086 mode, so user_mode(regs) can be used.
+ * user_mode(regs) determines whether a register set came from user
+ * mode.  On x86_32, this is true if V8086 mode was enabled OR if the
+ * register set was from protected mode with RPL-3 CS value.  This
+ * tricky test checks that with one comparison.
+ *
+ * On x86_64, vm86 mode is mercifully nonexistent, and we don't need
+ * the extra check.
  */
 static inline int user_mode(struct pt_regs *regs)
 {

@@ -113,12 +115,7 @@ static inline int user_mode(struct pt_regs *regs)
 
 static inline int user_mode_vm(struct pt_regs *regs)
 {
-#ifdef CONFIG_X86_32
-	return ((regs->cs & SEGMENT_RPL_MASK) | (regs->flags & X86_VM_MASK)) >=
-		USER_RPL;
-#else
 	return user_mode(regs);
-#endif
 }
 
 /*
merge bot
#25 lore raw

[tip:x86/asm] x86/asm/entry: Change all 'user_mode_vm()' calls to 'user_mode()'

In reply to Andy Lutomirski
x86/asm/entry: Change all 'user_mode_vm()' calls to 'user_mode()'
Commit-ID:  f39b6f0ef855a38ea17329a4e621ff97750dfcc2
Gitweb:     http://git.kernel.org/tip/f39b6f0ef855a38ea17329a4e621ff97750dfcc2
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:33 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:14:17 +0100

x86/asm/entry: Change all 'user_mode_vm()' calls to 'user_mode()'

user_mode_vm() and user_mode() are now the same. Change all callers of user_mode_vm() to user_mode().

The next patch will remove the definition of user_mode_vm.

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/43b1f57f3df70df5a08b0925897c660725015554.1426728647.git.luto@kernel.org open here
[ Merged to a more recent kernel. ]
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/alternative.c  |  2 +-
 arch/x86/kernel/crash.c        |  2 +-
 arch/x86/kernel/dumpstack.c    |  4 ++--
 arch/x86/kernel/dumpstack_32.c |  4 ++--
 arch/x86/kernel/i387.c         |  2 +-
 arch/x86/kernel/irq_32.c       |  2 +-
 arch/x86/kernel/irq_64.c       |  2 +-
 arch/x86/kernel/kgdb.c         |  4 ++--
 arch/x86/kernel/kprobes/core.c |  4 ++--
 arch/x86/kernel/process_32.c   |  2 +-
 arch/x86/kernel/ptrace.c       |  2 +-
 arch/x86/kernel/time.c         |  2 +-
 arch/x86/kernel/traps.c        | 16 ++++++++--------
 arch/x86/kernel/uprobes.c      |  2 +-
 arch/x86/mm/fault.c            |  6 +++---
 arch/x86/oprofile/backtrace.c  |  2 +-
 drivers/misc/sgi-xp/xpc_main.c |  2 +-
 17 files changed, 30 insertions(+), 30 deletions(-)
arch/x86/kernel/alternative.c +1 -1
index af397cc..5c993c9 100644
@@ -715,7 +715,7 @@ int poke_int3_handler(struct pt_regs *regs)
 	if (likely(!bp_patching_in_progress))
 		return 0;
 
-	if (user_mode_vm(regs) || regs->ip != (unsigned long)bp_int3_addr)
+	if (user_mode(regs) || regs->ip != (unsigned long)bp_int3_addr)
 		return 0;
 
 	/* set up the specified breakpoint handler */
arch/x86/kernel/crash.c +1 -1
index aceb2f9..c76d3e3 100644
@@ -105,7 +105,7 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
 #ifdef CONFIG_X86_32
 	struct pt_regs fixed_regs;
 
-	if (!user_mode_vm(regs)) {
+	if (!user_mode(regs)) {
 		crash_fixup_ss_esp(&fixed_regs, regs);
 		regs = &fixed_regs;
 	}
arch/x86/kernel/dumpstack.c +2 -2
index cf3df1d..ab3b656 100644
@@ -278,7 +278,7 @@ int __die(const char *str, struct pt_regs *regs, long err)
 	print_modules();
 	show_regs(regs);
 #ifdef CONFIG_X86_32
-	if (user_mode_vm(regs)) {
+	if (user_mode(regs)) {
 		sp = regs->sp;
 		ss = regs->ss & 0xffff;
 	} else {

@@ -307,7 +307,7 @@ void die(const char *str, struct pt_regs *regs, long err)
 	unsigned long flags = oops_begin();
 	int sig = SIGSEGV;
 
-	if (!user_mode_vm(regs))
+	if (!user_mode(regs))
 		report_bug(regs->ip, regs);
 
 	if (__die(str, regs, err))
arch/x86/kernel/dumpstack_32.c +2 -2
index 5abd4cd..39891ff 100644
@@ -123,13 +123,13 @@ void show_regs(struct pt_regs *regs)
 	int i;
 
 	show_regs_print_info(KERN_EMERG);
-	__show_regs(regs, !user_mode_vm(regs));
+	__show_regs(regs, !user_mode(regs));
 
 	/*
 	 * When in-kernel, we also print out the stack and code at the
 	 * time of the fault..
 	 */
-	if (!user_mode_vm(regs)) {
+	if (!user_mode(regs)) {
 		unsigned int code_prologue = code_bytes * 43 / 64;
 		unsigned int code_len = code_bytes;
 		unsigned char c;
arch/x86/kernel/i387.c +1 -1
index d5651fc..29c740d 100644
@@ -68,7 +68,7 @@ static inline bool interrupted_kernel_fpu_idle(void)
 static inline bool interrupted_user_mode(void)
 {
 	struct pt_regs *regs = get_irq_regs();
-	return regs && user_mode_vm(regs);
+	return regs && user_mode(regs);
 }
 
 /*
arch/x86/kernel/irq_32.c +1 -1
index 28d28f5..f9fd86a 100644
@@ -165,7 +165,7 @@ bool handle_irq(unsigned irq, struct pt_regs *regs)
 	if (unlikely(!desc))
 		return false;
 
-	if (user_mode_vm(regs) || !execute_on_irq_stack(overflow, desc, irq)) {
+	if (user_mode(regs) || !execute_on_irq_stack(overflow, desc, irq)) {
 		if (unlikely(overflow))
 			print_stack_overflow();
 		desc->handle_irq(irq, desc);
arch/x86/kernel/irq_64.c +1 -1
index e4b503d..394e643 100644
@@ -44,7 +44,7 @@ static inline void stack_overflow_check(struct pt_regs *regs)
 	u64 estack_top, estack_bottom;
 	u64 curbase = (u64)task_stack_page(current);
 
-	if (user_mode_vm(regs))
+	if (user_mode(regs))
 		return;
 
 	if (regs->sp >= curbase + sizeof(struct thread_info) +
arch/x86/kernel/kgdb.c +2 -2
index 7ec1d5f..7fe3a9d 100644
@@ -126,11 +126,11 @@ char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs)
 #ifdef CONFIG_X86_32
 	switch (regno) {
 	case GDB_SS:
-		if (!user_mode_vm(regs))
+		if (!user_mode(regs))
 			*(unsigned long *)mem = __KERNEL_DS;
 		break;
 	case GDB_SP:
-		if (!user_mode_vm(regs))
+		if (!user_mode(regs))
 			*(unsigned long *)mem = kernel_stack_pointer(regs);
 		break;
 	case GDB_GS:
arch/x86/kernel/kprobes/core.c +2 -2
index 4e3d5a9..24d0796 100644
@@ -602,7 +602,7 @@ int kprobe_int3_handler(struct pt_regs *regs)
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
 
-	if (user_mode_vm(regs))
+	if (user_mode(regs))
 		return 0;
 
 	addr = (kprobe_opcode_t *)(regs->ip - sizeof(kprobe_opcode_t));

@@ -1007,7 +1007,7 @@ int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val,
 	struct die_args *args = data;
 	int ret = NOTIFY_DONE;
 
-	if (args->regs && user_mode_vm(args->regs))
+	if (args->regs && user_mode(args->regs))
 		return ret;
 
 	if (val == DIE_GPF) {
arch/x86/kernel/process_32.c +1 -1
index 26c596d..c5e9870 100644
@@ -73,7 +73,7 @@ void __show_regs(struct pt_regs *regs, int all)
 	unsigned long sp;
 	unsigned short ss, gs;
 
-	if (user_mode_vm(regs)) {
+	if (user_mode(regs)) {
 		sp = regs->sp;
 		ss = regs->ss & 0xffff;
 		gs = get_user_gs(regs);
arch/x86/kernel/ptrace.c +1 -1
index 1e12581..a7bc794 100644
@@ -1415,7 +1415,7 @@ static void fill_sigtrap_info(struct task_struct *tsk,
 	memset(info, 0, sizeof(*info));
 	info->si_signo = SIGTRAP;
 	info->si_code = si_code;
-	info->si_addr = user_mode_vm(regs) ? (void __user *)regs->ip : NULL;
+	info->si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL;
 }
 
 void user_single_step_siginfo(struct task_struct *tsk,
arch/x86/kernel/time.c +1 -1
index 25adc0e..d39c091 100644
@@ -30,7 +30,7 @@ unsigned long profile_pc(struct pt_regs *regs)
 {
 	unsigned long pc = instruction_pointer(regs);
 
-	if (!user_mode_vm(regs) && in_lock_functions(pc)) {
+	if (!user_mode(regs) && in_lock_functions(pc)) {
 #ifdef CONFIG_FRAME_POINTER
 		return *(unsigned long *)(regs->bp + sizeof(long));
 #else
arch/x86/kernel/traps.c +8 -8
index 1136961..d4e2659 100644
@@ -112,7 +112,7 @@ enum ctx_state ist_enter(struct pt_regs *regs)
 {
 	enum ctx_state prev_state;
 
-	if (user_mode_vm(regs)) {
+	if (user_mode(regs)) {
 		/* Other than that, we're just an exception. */
 		prev_state = exception_enter();
 	} else {

@@ -146,7 +146,7 @@ void ist_exit(struct pt_regs *regs, enum ctx_state prev_state)
 	/* Must be before exception_exit. */
 	preempt_count_sub(HARDIRQ_OFFSET);
 
-	if (user_mode_vm(regs))
+	if (user_mode(regs))
 		return exception_exit(prev_state);
 	else
 		rcu_nmi_exit();

@@ -158,7 +158,7 @@ void ist_exit(struct pt_regs *regs, enum ctx_state prev_state)
  *
  * IST exception handlers normally cannot schedule.  As a special
  * exception, if the exception interrupted userspace code (i.e.
- * user_mode_vm(regs) would return true) and the exception was not
+ * user_mode(regs) would return true) and the exception was not
  * a double fault, it can be safe to schedule.  ist_begin_non_atomic()
  * begins a non-atomic section within an ist_enter()/ist_exit() region.
  * Callers are responsible for enabling interrupts themselves inside

@@ -167,7 +167,7 @@ void ist_exit(struct pt_regs *regs, enum ctx_state prev_state)
  */
 void ist_begin_non_atomic(struct pt_regs *regs)
 {
-	BUG_ON(!user_mode_vm(regs));
+	BUG_ON(!user_mode(regs));
 
 	/*
 	 * Sanity check: we need to be on the normal thread stack.  This

@@ -384,7 +384,7 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 		goto exit;
 	conditional_sti(regs);
 
-	if (!user_mode_vm(regs))
+	if (!user_mode(regs))
 		die("bounds", regs, error_code);
 
 	if (!cpu_feature_enabled(X86_FEATURE_MPX)) {

@@ -587,7 +587,7 @@ struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
 	/* Copy the remainder of the stack from the current stack. */
 	memmove(new_stack, s, offsetof(struct bad_iret_stack, regs.ip));
 
-	BUG_ON(!user_mode_vm(&new_stack->regs));
+	BUG_ON(!user_mode(&new_stack->regs));
 	return new_stack;
 }
 NOKPROBE_SYMBOL(fixup_bad_iret);

@@ -637,7 +637,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	 * then it's very likely the result of an icebp/int01 trap.
 	 * User wants a sigtrap for that.
 	 */
-	if (!dr6 && user_mode_vm(regs))
+	if (!dr6 && user_mode(regs))
 		user_icebp = 1;
 
 	/* Catch kmemcheck conditions first of all! */

@@ -721,7 +721,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 		return;
 	conditional_sti(regs);
 
-	if (!user_mode_vm(regs))
+	if (!user_mode(regs))
 	{
 		if (!fixup_exception(regs)) {
 			task->thread.error_code = error_code;
arch/x86/kernel/uprobes.c +1 -1
index 81f8adb0..0b81ad6 100644
@@ -912,7 +912,7 @@ int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val,
 	int ret = NOTIFY_DONE;
 
 	/* We are only interested in userspace traps */
-	if (regs && !user_mode_vm(regs))
+	if (regs && !user_mode(regs))
 		return NOTIFY_DONE;
 
 	switch (val) {
arch/x86/mm/fault.c +3 -3
index ae340d3..181c53b 100644
@@ -59,7 +59,7 @@ static nokprobe_inline int kprobes_fault(struct pt_regs *regs)
 	int ret = 0;
 
 	/* kprobe_running() needs smp_processor_id() */
-	if (kprobes_built_in() && !user_mode_vm(regs)) {
+	if (kprobes_built_in() && !user_mode(regs)) {
 		preempt_disable();
 		if (kprobe_running() && kprobe_fault_handler(regs, 14))
 			ret = 1;

@@ -1035,7 +1035,7 @@ static inline bool smap_violation(int error_code, struct pt_regs *regs)
 	if (error_code & PF_USER)
 		return false;
 
-	if (!user_mode_vm(regs) && (regs->flags & X86_EFLAGS_AC))
+	if (!user_mode(regs) && (regs->flags & X86_EFLAGS_AC))
 		return false;
 
 	return true;

@@ -1140,7 +1140,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
 	 * User-mode registers count as a user access even for any
 	 * potential system fault or CPU buglet:
 	 */
-	if (user_mode_vm(regs)) {
+	if (user_mode(regs)) {
 		local_irq_enable();
 		error_code |= PF_USER;
 		flags |= FAULT_FLAG_USER;
arch/x86/oprofile/backtrace.c +1 -1
index 5d04be5..4e664bd 100644
@@ -111,7 +111,7 @@ x86_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
 	struct stack_frame *head = (struct stack_frame *)frame_pointer(regs);
 
-	if (!user_mode_vm(regs)) {
+	if (!user_mode(regs)) {
 		unsigned long stack = kernel_stack_pointer(regs);
 		if (depth)
 			dump_trace(NULL, regs, (unsigned long *)stack, 0,
drivers/misc/sgi-xp/xpc_main.c +1 -1
index 82dc574..7f32712 100644
@@ -1210,7 +1210,7 @@ xpc_system_die(struct notifier_block *nb, unsigned long event, void *_die_args)
 
 		if (((die_args->trapnr == X86_TRAP_MF) ||
 		     (die_args->trapnr == X86_TRAP_XF)) &&
-		    !user_mode_vm(die_args->regs))
+		    !user_mode(die_args->regs))
 			xpc_die_deactivate();
 
 		break;
merge bot
#27 lore raw

[tip:x86/asm] x86/asm/entry: Remove user_mode_vm()

In reply to Andy Lutomirski
x86/asm/entry: Remove user_mode_vm()
Commit-ID:  7a2806741e7327a6b20ccef42e8d56588cb2fef5
Gitweb:     http://git.kernel.org/tip/7a2806741e7327a6b20ccef42e8d56588cb2fef5
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:34 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:14:33 +0100

x86/asm/entry: Remove user_mode_vm()

It has no callers anymore.

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/a594afd6a0bddb1311bd7c92a15201c87fbb8681.1426728647.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/include/asm/ptrace.h | 5 -----
 1 file changed, 5 deletions(-)
arch/x86/include/asm/ptrace.h +0 -5
index 70c439f..d20bae2 100644
@@ -113,11 +113,6 @@ static inline int user_mode(struct pt_regs *regs)
 #endif
 }
 
-static inline int user_mode_vm(struct pt_regs *regs)
-{
-	return user_mode(regs);
-}
-
 /*
  * This is the fastest way to check whether regs come from user space.
  * It is unsafe if regs might come from vm86 mode, though -- in vm86
merge bot
#29 lore raw

[tip:x86/asm] x86/asm/entry: Replace some open-coded VM86 checks with v8086_mode() checks

In reply to Andy Lutomirski
x86/asm/entry: Replace some open-coded VM86 checks with v8086_mode() checks
Commit-ID:  d74ef1118a146ae1135c8b26fff2bfee980fd7a4
Gitweb:     http://git.kernel.org/tip/d74ef1118a146ae1135c8b26fff2bfee980fd7a4
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:35 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:14:40 +0100

x86/asm/entry: Replace some open-coded VM86 checks with v8086_mode() checks

This allows us to remove some unnecessary ifdefs. There should be no change to the generated code.

Signed-off-by Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/f7e00f0d668e253abf0bd8bf36491ac47bd761ff.1426728647.git.luto@kernel.org open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/traps.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
arch/x86/kernel/traps.c +4 -7
index d4e2659..c8eb469 100644
@@ -194,8 +194,7 @@ static nokprobe_inline int
 do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		  struct pt_regs *regs,	long error_code)
 {
-#ifdef CONFIG_X86_32
-	if (regs->flags & X86_VM_MASK) {
+	if (v8086_mode(regs)) {
 		/*
 		 * Traps 0, 1, 3, 4, and 5 should be forwarded to vm86.
 		 * On nmi (interrupt 2), do_trap should not be called.

@@ -207,7 +206,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		}
 		return -1;
 	}
-#endif
+
 	if (!user_mode_ignore_vm86(regs)) {
 		if (!fixup_exception(regs)) {
 			tsk->thread.error_code = error_code;

@@ -462,13 +461,11 @@ do_general_protection(struct pt_regs *regs, long error_code)
 	prev_state = exception_enter();
 	conditional_sti(regs);
 
-#ifdef CONFIG_X86_32
-	if (regs->flags & X86_VM_MASK) {
+	if (v8086_mode(regs)) {
 		local_irq_enable();
 		handle_vm86_fault((struct kernel_vm86_regs *) regs, error_code);
 		goto exit;
 	}
-#endif
 
 	tsk = current;
 	if (!user_mode_ignore_vm86(regs)) {

@@ -673,7 +670,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	/* It's safe to allow irq's after DR6 has been saved */
 	preempt_conditional_sti(regs);
 
-	if (regs->flags & X86_VM_MASK) {
+	if (v8086_mode(regs)) {
 		handle_vm86_trap((struct kernel_vm86_regs *) regs, error_code,
 					X86_TRAP_DB);
 		preempt_conditional_cli(regs);
#7 lore raw

Re: [tip:x86/asm] x86/asm/entry: Add user_mode_ignore_vm86()

In reply to tip-bot for Andy Lutomirski
Quoted 9 lines · tip-bot for Andy Lutomirski
Commit-ID:  a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Gitweb:     http://git.kernel.org/tip/a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:29 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:13:36 +0100

x86/asm/entry: Add user_mode_ignore_vm86()

Quoted 7 lines

user_mode() is dangerous and user_mode_vm() has a confusing name.

Add user_mode_ignore_vm86() (equivalent to current user_mode()). We'll change the small number of legitimate users of user_mode() to user_mode_ignore_vm86().

Inspired by grsec, although this works rather differently.

Ingo, does this mean that you changed your mind or do you still want a patch to delete user_mode_ignore_vm86 and just use user_mode everywhere instead?

--Andy

#8 lore raw

Re: [tip:x86/asm] x86/asm/entry: Add user_mode_ignore_vm86()

Quoted 17 lines · Andy Lutomirski
Quoted 15 lines · tip-bot for Andy Lutomirski
Commit-ID:  a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Gitweb:     http://git.kernel.org/tip/a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:29 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:13:36 +0100

x86/asm/entry: Add user_mode_ignore_vm86()

user_mode() is dangerous and user_mode_vm() has a confusing name.

Add user_mode_ignore_vm86() (equivalent to current user_mode()). We'll change the small number of legitimate users of user_mode() to user_mode_ignore_vm86().

Quoted 5 lines
Quoted 1 line

Inspired by grsec, although this works rather differently.

Ingo, does this mean that you changed your mind or do you still want a patch to delete user_mode_ignore_vm86 and just use user_mode everywhere instead?

Would be still nice to have it as an add on patch, if you agree with my arguments.

I picked up your series as-is because it's correct and because it already improves things a lot in this area.

Thanks,

Ingo

#9 lore raw

Re: [tip:x86/asm] x86/asm/entry: Add user_mode_ignore_vm86()

Quoted 20 lines · Ingo Molnar
Quoted 17 lines · Andy Lutomirski
Quoted 15 lines · tip-bot for Andy Lutomirski
Commit-ID:  a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Gitweb:     http://git.kernel.org/tip/a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:29 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:13:36 +0100

x86/asm/entry: Add user_mode_ignore_vm86()

user_mode() is dangerous and user_mode_vm() has a confusing name.

Add user_mode_ignore_vm86() (equivalent to current user_mode()). We'll change the small number of legitimate users of user_mode() to user_mode_ignore_vm86().

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Inspired by grsec, although this works rather differently.

Ingo, does this mean that you changed your mind or do you still want a patch to delete user_mode_ignore_vm86 and just use user_mode everywhere instead?

Would be still nice to have it as an add on patch, if you agree with my arguments.

Given that there are only a very small number of callers left and they're all Obviously Correct (tm), I'm not too worried about it. Maybe if we kill off __copy_to_user, I'll be inspired to kill off user_mode_ignore_vm86 as well :)

--Andy

Quoted 7 lines

I picked up your series as-is because it's correct and because it already improves things a lot in this area.

Thanks,

Ingo

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
#10 lore raw

Re: [tip:x86/asm] x86/asm/entry: Add user_mode_ignore_vm86()

Quoted 27 lines · Andy Lutomirski
Quoted 26 lines · Ingo Molnar
Quoted 22 lines · Andy Lutomirski
Quoted 9 lines · tip-bot for Andy Lutomirski
Commit-ID:  a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Gitweb:     http://git.kernel.org/tip/a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:29 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:13:36 +0100

x86/asm/entry: Add user_mode_ignore_vm86()

Quoted 7 lines

user_mode() is dangerous and user_mode_vm() has a confusing name.

Add user_mode_ignore_vm86() (equivalent to current user_mode()). We'll change the small number of legitimate users of user_mode() to user_mode_ignore_vm86().

Inspired by grsec, although this works rather differently.

Ingo, does this mean that you changed your mind or do you still want a patch to delete user_mode_ignore_vm86 and just use user_mode everywhere instead?

Quoted 7 lines
Quoted 2 lines

Would be still nice to have it as an add on patch, if you agree with my arguments.

Given that there are only a very small number of callers left and they're all Obviously Correct (tm), I'm not too worried about it. Maybe if we kill off __copy_to_user, I'll be inspired to kill off user_mode_ignore_vm86 as well :)

I was looking at the code involving this function and it looks like a much better name for user_mode_ignore_vm86() would be user_mode_cs().

Every time we use it, we check vm8086 mode just before it:

perf_event.c

        if (regs->flags & X86_VM_MASK)
                return 0x10 * regs->cs;

        if (user_mode_ignore_vm86(regs) && regs->cs != __USER_CS)
                return get_segment_base(regs->cs);

traps.c (three similar instances):

        if (v8086_mode(regs)) {
...
                goto exit;
        }
        if (user_mode_ignore_vm86(regs))...

"_ignore_vm86" part doesn't quite work as an explanation. user_mode_cs() would immediately tell me "do we have a user's cs?"

#11 lore raw

Re: [tip:x86/asm] x86/asm/entry: Add user_mode_ignore_vm86()

Quoted 61 lines · Denys Vlasenko
Quoted 27 lines · Andy Lutomirski
Quoted 26 lines · Ingo Molnar
Quoted 22 lines · Andy Lutomirski
Quoted 9 lines · tip-bot for Andy Lutomirski
Commit-ID:  a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Gitweb:     http://git.kernel.org/tip/a67e7277d01ccfd39b0db5a198c2643cc19dd79c
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Wed, 18 Mar 2015 18:33:29 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 11:13:36 +0100

x86/asm/entry: Add user_mode_ignore_vm86()

Quoted 7 lines

user_mode() is dangerous and user_mode_vm() has a confusing name.

Add user_mode_ignore_vm86() (equivalent to current user_mode()). We'll change the small number of legitimate users of user_mode() to user_mode_ignore_vm86().

Inspired by grsec, although this works rather differently.

Ingo, does this mean that you changed your mind or do you still want a patch to delete user_mode_ignore_vm86 and just use user_mode everywhere instead?

Quoted 7 lines
Quoted 2 lines

Would be still nice to have it as an add on patch, if you agree with my arguments.

Given that there are only a very small number of callers left and they're all Obviously Correct (tm), I'm not too worried about it. Maybe if we kill off __copy_to_user, I'll be inspired to kill off user_mode_ignore_vm86 as well :)

I was looking at the code involving this function and it looks like a much better name for user_mode_ignore_vm86() would be user_mode_cs().

Every time we use it, we check vm8086 mode just before it:

perf_event.c

        if (regs->flags & X86_VM_MASK)
                return 0x10 * regs->cs;

        if (user_mode_ignore_vm86(regs) && regs->cs != __USER_CS)
                return get_segment_base(regs->cs);

traps.c (three similar instances):

        if (v8086_mode(regs)) {
...
                goto exit;
        }
        if (user_mode_ignore_vm86(regs))...
Quoted 2 lines

"_ignore_vm86" part doesn't quite work as an explanation. user_mode_cs() would immediately tell me "do we have a user's cs?"

So what the function name wanted to express is something like this:

	if (user_mode_vm86_mode_already_checked_so_this_is_marginally_faster_but_dont_use_it_otherwise_because_that_would_be_a_roothole()) 
	{
		...
	}

but that name was considered somewhat long.

Thanks,

Ingo

#12 lore raw

PATCH 1/1 x86/asm/entry: Remove user_mode_ignore_vm86()

Quoted 8 lines · Ingo Molnar

So what the function name wanted to express is something like this:

	if (user_mode_vm86_mode_already_checked_so_this_is_marginally_faster_but_dont_use_it_otherwise_because_that_would_be_a_roothole()) 
	{
		...
	}

but that name was considered somewhat long.

So how about doing the patch below?

Thanks,

Ingo

===================================>

Quoted 1 line

From 6677d6f073cfda7f1036eb06d13faaad5c6742cc Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
Date: Sun, 29 Mar 2015 09:10:08 +0200
Subject: [PATCH] x86/asm/entry: Remove user_mode_ignore_vm86()

user_mode_ignore_vm86() can be used instead of user_mode(), in places where we have already done a v8086_mode() security check of ptregs.

But doing this check in the wrong place would be a bug that could result in security problems, and also the naming still isn't very clear.

Furthermore, it only affects 32-bit kernels, while most development happens on 64-bit kernels.

If we replace them with user_mode() checks then the cost is only a very minor increase in various slowpaths:

   text             data   bss     dec              hex    filename
   10573391         703562 1753042 13029995         c6d26b vmlinux.o.before
   10573423         703562 1753042 13030027         c6d28b vmlinux.o.after

So lets get rid of this distinction once and for all.

Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Oleg Nesterov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/include/asm/ptrace.h    | 17 -----------------
 arch/x86/kernel/cpu/perf_event.c |  2 +-
 arch/x86/kernel/traps.c          |  6 +++---
 3 files changed, 4 insertions(+), 21 deletions(-)
arch/x86/include/asm/ptrace.h +0 -17
index d20bae298852..19507ffa5d28 100644
@@ -113,23 +113,6 @@ static inline int user_mode(struct pt_regs *regs)
 #endif
 }
 
-/*
- * This is the fastest way to check whether regs come from user space.
- * It is unsafe if regs might come from vm86 mode, though -- in vm86
- * mode, all bits of CS and SS are completely under the user's control.
- * The CPU considers vm86 mode to be CPL 3 regardless of CS and SS.
- *
- * Do NOT use this function unless you have already ruled out the
- * possibility that regs came from vm86 mode.
- *
- * We check for RPL != 0 instead of RPL == 3 because we don't use rings
- * 1 or 2 and this is more efficient.
- */
-static inline int user_mode_ignore_vm86(struct pt_regs *regs)
-{
-	return (regs->cs & SEGMENT_RPL_MASK) != 0;
-}
-
 static inline int v8086_mode(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_32
arch/x86/kernel/cpu/perf_event.c +1 -1
index 56f7e60ad732..e2888a3ad1e3 100644
@@ -2159,7 +2159,7 @@ static unsigned long code_segment_base(struct pt_regs *regs)
 	if (regs->flags & X86_VM_MASK)
 		return 0x10 * regs->cs;
 
-	if (user_mode_ignore_vm86(regs) && regs->cs != __USER_CS)
+	if (user_mode(regs) && regs->cs != __USER_CS)
 		return get_segment_base(regs->cs);
 #else
 	if (user_mode(regs) && !user_64bit_mode(regs) &&
arch/x86/kernel/traps.c +3 -3
index c8eb469a94a4..6751c5c58eec 100644
@@ -207,7 +207,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		return -1;
 	}
 
-	if (!user_mode_ignore_vm86(regs)) {
+	if (!user_mode(regs)) {
 		if (!fixup_exception(regs)) {
 			tsk->thread.error_code = error_code;
 			tsk->thread.trap_nr = trapnr;

@@ -468,7 +468,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
 	}
 
 	tsk = current;
-	if (!user_mode_ignore_vm86(regs)) {
+	if (!user_mode(regs)) {
 		if (fixup_exception(regs))
 			goto exit;
 

@@ -685,7 +685,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	 * We already checked v86 mode above, so we can check for kernel mode
 	 * by just checking the CPL of CS.
 	 */
-	if ((dr6 & DR_STEP) && !user_mode_ignore_vm86(regs)) {
+	if ((dr6 & DR_STEP) && !user_mode(regs)) {
 		tsk->thread.debugreg6 &= ~DR_STEP;
 		set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
 		regs->flags &= ~X86_EFLAGS_TF;
maintainer
#16 lore raw

Re: [tip:x86/asm] x86/asm/entry: Add user_mode_ignore_vm86()

In reply to Ingo Molnar
Quoted 3 lines · Ingo Molnar

So what the function name wanted to express is something like this:

if (user_mode_vm86_mode_already_checked_so_this_is_marginally_faster_but_dont_use_it_otherwise_because_that_would_be_a_roothole())

LOL.

This wins the categories Longest Function Name of the Year and Most Descriptive Function Name of the Year!

Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
maintainer Acked-by on v1 1/1
#13 lore raw

Re: [PATCH] x86/asm/entry: Remove user_mode_ignore_vm86()

In reply to Ingo Molnar
Quoted 39 lines · Ingo Molnar

So how about doing the patch below?

Thanks,

Ingo

===================================>
From 6677d6f073cfda7f1036eb06d13faaad5c6742cc Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Sun, 29 Mar 2015 09:10:08 +0200
Subject: [PATCH] x86/asm/entry: Remove user_mode_ignore_vm86()

user_mode_ignore_vm86() can be used instead of user_mode(), in places where we have already done a v8086_mode() security check of ptregs.

But doing this check in the wrong place would be a bug that could result in security problems, and also the naming still isn't very clear.

Furthermore, it only affects 32-bit kernels, while most development happens on 64-bit kernels.

If we replace them with user_mode() checks then the cost is only a very minor increase in various slowpaths:

   text             data   bss     dec              hex    filename
   10573391         703562 1753042 13029995         c6d26b vmlinux.o.before
   10573423         703562 1753042 13030027         c6d28b vmlinux.o.after

So lets get rid of this distinction once and for all.

Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Oleg Nesterov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
Quoted 5 lines
---
 arch/x86/include/asm/ptrace.h    | 17 -----------------
 arch/x86/kernel/cpu/perf_event.c |  2 +-
 arch/x86/kernel/traps.c          |  6 +++---
 3 files changed, 4 insertions(+), 21 deletions(-)

I had some doubts about people using user_mode_ignore_vm86() in the wrong way and thus introducing sec. bugs.

Since this is only on the slow path, simplifying the code makes sense to me.

Acked-by Borislav Petkov <[email protected]>
Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
Acked-by
#14 lore raw

Re: [PATCH] x86/asm/entry: Remove user_mode_ignore_vm86()

Quoted 46 lines · Borislav Petkov
Quoted 39 lines · Ingo Molnar

So how about doing the patch below?

Thanks,

Ingo

===================================>
From 6677d6f073cfda7f1036eb06d13faaad5c6742cc Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Sun, 29 Mar 2015 09:10:08 +0200
Subject: [PATCH] x86/asm/entry: Remove user_mode_ignore_vm86()

user_mode_ignore_vm86() can be used instead of user_mode(), in places where we have already done a v8086_mode() security check of ptregs.

But doing this check in the wrong place would be a bug that could result in security problems, and also the naming still isn't very clear.

Furthermore, it only affects 32-bit kernels, while most development happens on 64-bit kernels.

If we replace them with user_mode() checks then the cost is only a very minor increase in various slowpaths:

   text             data   bss     dec              hex    filename
   10573391         703562 1753042 13029995         c6d26b vmlinux.o.before
   10573423         703562 1753042 13030027         c6d28b vmlinux.o.after

So lets get rid of this distinction once and for all.

Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Oleg Nesterov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
Quoted 5 lines
---
 arch/x86/include/asm/ptrace.h    | 17 -----------------
 arch/x86/kernel/cpu/perf_event.c |  2 +-
 arch/x86/kernel/traps.c          |  6 +++---
 3 files changed, 4 insertions(+), 21 deletions(-)
Quoted 8 lines

I had some doubts about people using user_mode_ignore_vm86() in the wrong way and thus introducing sec. bugs.

Since this is only on the slow path, simplifying the code makes sense to me.

Acked-by Borislav Petkov <[email protected]>

Ditto.

Acked-by Andy Lutomirski <[email protected]>
Quoted 6 lines

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
#17 lore raw

Re: [tip:x86/asm] x86/asm/entry: Add user_mode_ignore_vm86()

In reply to Ingo Molnar
Quoted 36 lines · Ingo Molnar
Quoted 33 lines
Quoted 7 lines
Quoted 2 lines

Would be still nice to have it as an add on patch, if you agree with my arguments.

Given that there are only a very small number of callers left and they're all Obviously Correct (tm), I'm not too worried about it. Maybe if we kill off __copy_to_user, I'll be inspired to kill off user_mode_ignore_vm86 as well :)

I was looking at the code involving this function and it looks like a much better name for user_mode_ignore_vm86() would be user_mode_cs().

Every time we use it, we check vm8086 mode just before it:

perf_event.c

        if (regs->flags & X86_VM_MASK)
                return 0x10 * regs->cs;

        if (user_mode_ignore_vm86(regs) && regs->cs != __USER_CS)
                return get_segment_base(regs->cs);

traps.c (three similar instances):

        if (v8086_mode(regs)) {
...
                goto exit;
        }
        if (user_mode_ignore_vm86(regs))...
Quoted 2 lines

"_ignore_vm86" part doesn't quite work as an explanation. user_mode_cs() would immediately tell me "do we have a user's cs?"

Quoted 8 lines

So what the function name wanted to express is something like this:

        if (user_mode_vm86_mode_already_checked_so_this_is_marginally_faster_but_dont_use_it_otherwise_because_that_would_be_a_roothole())
        {
                ...
        }

but that name was considered somewhat long.

LOL :D

Seriously, though. I do think that user_mode_cs(regs) is a good name. It's short.
It describes what it in fact checks.
"(is it) user mode cs" reads as a valid English phrase, whereas "(is it) user mode ignore vm86" does not.

merge bot
#15 lore raw

[tip:x86/asm] x86/asm/entry: Remove user_mode_ignore_vm86()

In reply to Ingo Molnar
x86/asm/entry: Remove user_mode_ignore_vm86()
Commit-ID:  55474c48b4726fd3914c1ec47fced0f931729979
Gitweb:     http://git.kernel.org/tip/55474c48b4726fd3914c1ec47fced0f931729979
Author:     Ingo Molnar <[email protected]>
AuthorDate: Sun, 29 Mar 2015 11:02:34 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 31 Mar 2015 11:45:19 +0200

x86/asm/entry: Remove user_mode_ignore_vm86()

user_mode_ignore_vm86() can be used instead of user_mode(), in places where we have already done a v8086_mode() security check of ptregs.

But doing this check in the wrong place would be a bug that could result in security problems, and also the naming still isn't very clear.

Furthermore, it only affects 32-bit kernels, while most development happens on 64-bit kernels.

If we replace them with user_mode() checks then the cost is only a very minor increase in various slowpaths:

   text             data   bss     dec              hex    filename
   10573391         703562 1753042 13029995         c6d26b vmlinux.o.before
   10573423         703562 1753042 13030027         c6d28b vmlinux.o.after

So lets get rid of this distinction once and for all.

Acked-by Borislav Petkov <[email protected]>
Acked-by Andy Lutomirski <[email protected]>
Cc Andrew Lutomirski <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brad Spengler <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Oleg Nesterov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/include/asm/ptrace.h    | 17 -----------------
 arch/x86/kernel/cpu/perf_event.c |  2 +-
 arch/x86/kernel/traps.c          |  6 +++---
 3 files changed, 4 insertions(+), 21 deletions(-)
arch/x86/include/asm/ptrace.h +0 -17
index d20bae2..19507ff 100644
@@ -113,23 +113,6 @@ static inline int user_mode(struct pt_regs *regs)
 #endif
 }
 
-/*
- * This is the fastest way to check whether regs come from user space.
- * It is unsafe if regs might come from vm86 mode, though -- in vm86
- * mode, all bits of CS and SS are completely under the user's control.
- * The CPU considers vm86 mode to be CPL 3 regardless of CS and SS.
- *
- * Do NOT use this function unless you have already ruled out the
- * possibility that regs came from vm86 mode.
- *
- * We check for RPL != 0 instead of RPL == 3 because we don't use rings
- * 1 or 2 and this is more efficient.
- */
-static inline int user_mode_ignore_vm86(struct pt_regs *regs)
-{
-	return (regs->cs & SEGMENT_RPL_MASK) != 0;
-}
-
 static inline int v8086_mode(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_32
arch/x86/kernel/cpu/perf_event.c +1 -1
index 56f7e60..e2888a3 100644
@@ -2159,7 +2159,7 @@ static unsigned long code_segment_base(struct pt_regs *regs)
 	if (regs->flags & X86_VM_MASK)
 		return 0x10 * regs->cs;
 
-	if (user_mode_ignore_vm86(regs) && regs->cs != __USER_CS)
+	if (user_mode(regs) && regs->cs != __USER_CS)
 		return get_segment_base(regs->cs);
 #else
 	if (user_mode(regs) && !user_64bit_mode(regs) &&
arch/x86/kernel/traps.c +3 -3
index c8eb469..6751c5c 100644
@@ -207,7 +207,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		return -1;
 	}
 
-	if (!user_mode_ignore_vm86(regs)) {
+	if (!user_mode(regs)) {
 		if (!fixup_exception(regs)) {
 			tsk->thread.error_code = error_code;
 			tsk->thread.trap_nr = trapnr;

@@ -468,7 +468,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
 	}
 
 	tsk = current;
-	if (!user_mode_ignore_vm86(regs)) {
+	if (!user_mode(regs)) {
 		if (fixup_exception(regs))
 			goto exit;
 

@@ -685,7 +685,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	 * We already checked v86 mode above, so we can check for kernel mode
 	 * by just checking the CPL of CS.
 	 */
-	if ((dr6 & DR_STEP) && !user_mode_ignore_vm86(regs)) {
+	if ((dr6 & DR_STEP) && !user_mode(regs)) {
 		tsk->thread.debugreg6 &= ~DR_STEP;
 		set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
 		regs->flags &= ~X86_EFLAGS_TF;