[PATCH 0/5] x86/vm86: First round of vm86 cleanups

[PATCH 0/5] x86/vm86: First round of vm86 cleanups

[Brian Gerst]

The goal of this set of patches is to change vm86 support to return to
userspace with the normal exit paths instead of leaving data on the kernel
stack and jumping directly into the exit asm routines.  This fixes issues 
like ptrace and syscall auditing not working with vm86, and makes possible
cleanups in the syscall exit work code.

 arch/x86/entry/entry_32.S          |  24 +---
 arch/x86/include/asm/processor.h   |  11 +-
 arch/x86/include/asm/thread_info.h |  11 +-
 arch/x86/include/asm/vm86.h        |  36 ++---
 arch/x86/kernel/process.c          |   7 +
 arch/x86/kernel/signal.c           |   3 +
 arch/x86/kernel/vm86_32.c          | 265 ++++++++++++++++---------------------
 arch/x86/mm/fault.c                |   4 +-
 8 files changed, 152 insertions(+), 209 deletions(-)

[PATCH 1/5] x86/vm86: Move userspace accesses to do_sys_vm86()

[Brian Gerst]

Move the userspace accesses down into the common function in
preparation for the next set of patches.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/kernel/vm86_32.c | 61 +++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index fc9db6e..71a8b0a 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -200,7 +200,8 @@ out:
 
 
 static int do_vm86_irq_handling(int subfunction, int irqnumber);
-static void do_sys_vm86(struct kernel_vm86_struct *info, struct task_struct *tsk);
+static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
+			struct kernel_vm86_struct *info);
 
 SYSCALL_DEFINE1(vm86old, struct vm86_struct __user *, v86)
 {
@@ -209,21 +210,8 @@ SYSCALL_DEFINE1(vm86old, struct vm86_struct __user *, v86)
 					 * This remains on the stack until we
 					 * return to 32 bit user space.
 					 */
-	struct task_struct *tsk = current;
-	int tmp;
 
-	if (tsk->thread.saved_sp0)
-		return -EPERM;
-	tmp = copy_vm86_regs_from_user(&info.regs, &v86->regs,
-				       offsetof(struct kernel_vm86_struct, vm86plus) -
-				       sizeof(info.regs));
-	if (tmp)
-		return -EFAULT;
-	memset(&info.vm86plus, 0, (int)&info.regs32 - (int)&info.vm86plus);
-	info.regs32 = current_pt_regs();
-	tsk->thread.vm86_info = v86;
-	do_sys_vm86(&info, tsk);
-	return 0;	/* we never return here */
+	return do_sys_vm86((struct vm86plus_struct __user *) v86, false, &info);
 }
 
 
@@ -234,11 +222,7 @@ SYSCALL_DEFINE2(vm86, unsigned long, cmd, unsigned long, arg)
 					 * This remains on the stack until we
 					 * return to 32 bit user space.
 					 */
-	struct task_struct *tsk;
-	int tmp;
-	struct vm86plus_struct __user *v86;
 
-	tsk = current;
 	switch (cmd) {
 	case VM86_REQUEST_IRQ:
 	case VM86_FREE_IRQ:
@@ -256,25 +240,34 @@ SYSCALL_DEFINE2(vm86, unsigned long, cmd, unsigned long, arg)
 	}
 
 	/* we come here only for functions VM86_ENTER, VM86_ENTER_NO_BYPASS */
-	if (tsk->thread.saved_sp0)
-		return -EPERM;
-	v86 = (struct vm86plus_struct __user *)arg;
-	tmp = copy_vm86_regs_from_user(&info.regs, &v86->regs,
-				       offsetof(struct kernel_vm86_struct, regs32) -
-				       sizeof(info.regs));
-	if (tmp)
-		return -EFAULT;
-	info.regs32 = current_pt_regs();
-	info.vm86plus.is_vm86pus = 1;
-	tsk->thread.vm86_info = (struct vm86_struct __user *)v86;
-	do_sys_vm86(&info, tsk);
-	return 0;	/* we never return here */
+	return do_sys_vm86((struct vm86plus_struct __user *) arg, true, &info);
 }
 
 
-static void do_sys_vm86(struct kernel_vm86_struct *info, struct task_struct *tsk)
+static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
+			struct kernel_vm86_struct *info)
 {
 	struct tss_struct *tss;
+	struct task_struct *tsk = current;
+
+	if (tsk->thread.saved_sp0)
+		return -EPERM;
+	if (plus) {
+		if (copy_vm86_regs_from_user(&info->regs, &v86->regs,
+			offsetof(struct kernel_vm86_struct, regs32) -
+			sizeof(info->regs)))
+			return -EFAULT;
+		info->vm86plus.is_vm86pus = 1;
+	} else {
+		if (copy_vm86_regs_from_user(&info->regs, &v86->regs,
+			offsetof(struct kernel_vm86_struct, vm86plus) -
+			sizeof(info->regs)))
+			return -EFAULT;
+		memset(&info->vm86plus, 0, sizeof(struct vm86plus_info_struct));
+	}
+	info->regs32 = current_pt_regs();
+	tsk->thread.vm86_info = (struct vm86_struct __user *) v86;
+
 /*
  * make sure the vm86() system call doesn't try to do anything silly
  */
@@ -344,7 +337,7 @@ static void do_sys_vm86(struct kernel_vm86_struct *info, struct task_struct *tsk
 		"jmp resume_userspace"
 		: /* no outputs */
 		:"r" (&info->regs), "r" (task_thread_info(tsk)), "r" (0));
-	/* we never return here */
+	return 0;	/* we never return here */
 }
 
 static inline void return_to_32bit(struct kernel_vm86_regs *regs16, int retval)
-- 
2.4.3

[PATCH 2/5] x86/vm86: Move vm86 fields out of thread_struct

[Brian Gerst]

Allocate a separate structure for the vm86 fields.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/include/asm/processor.h | 11 ++------
 arch/x86/include/asm/vm86.h      | 10 ++++++++
 arch/x86/kernel/process.c        |  7 +++++
 arch/x86/kernel/vm86_32.c        | 55 +++++++++++++++++++++++-----------------
 arch/x86/mm/fault.c              |  4 +--
 5 files changed, 53 insertions(+), 34 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 43e6519..8085463 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -403,15 +403,9 @@ struct thread_struct {
 	unsigned long		cr2;
 	unsigned long		trap_nr;
 	unsigned long		error_code;
-#ifdef CONFIG_X86_32
+#ifdef CONFIG_VM86
 	/* Virtual 86 mode info */
-	struct vm86_struct __user *vm86_info;
-	unsigned long		screen_bitmap;
-	unsigned long		v86flags;
-	unsigned long		v86mask;
-	unsigned long		saved_sp0;
-	unsigned int		saved_fs;
-	unsigned int		saved_gs;
+	struct kernel_vm86_info	*vm86;
 #endif
 	/* IO permissions: */
 	unsigned long		*io_bitmap_ptr;
@@ -716,7 +710,6 @@ static inline void spin_lock_prefetch(const void *x)
 
 #define INIT_THREAD  {							  \
 	.sp0			= TOP_OF_INIT_STACK,			  \
-	.vm86_info		= NULL,					  \
 	.sysenter_cs		= __KERNEL_CS,				  \
 	.io_bitmap_ptr		= NULL,					  \
 }
diff --git a/arch/x86/include/asm/vm86.h b/arch/x86/include/asm/vm86.h
index 1d8de3f..88f14e3 100644
--- a/arch/x86/include/asm/vm86.h
+++ b/arch/x86/include/asm/vm86.h
@@ -58,6 +58,16 @@ struct kernel_vm86_struct {
  */
 };
 
+struct kernel_vm86_info {
+	struct vm86_struct __user *vm86_info;
+	unsigned long screen_bitmap;
+	unsigned long v86flags;
+	unsigned long v86mask;
+	unsigned long saved_sp0;
+	unsigned int saved_fs;
+	unsigned int saved_gs;
+};
+
 #ifdef CONFIG_VM86
 
 void handle_vm86_fault(struct kernel_vm86_regs *, long);
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 9cad694..5dcd037 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -110,6 +110,13 @@ void exit_thread(void)
 		kfree(bp);
 	}
 
+#ifdef CONFIG_VM86
+	if (t->vm86) {
+		kfree(t->vm86);
+		t->vm86 = NULL;
+	}
+#endif
+
 	fpu__drop(fpu);
 }
 
diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index 71a8b0a..8b29c9b 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -44,6 +44,7 @@
 #include <linux/ptrace.h>
 #include <linux/audit.h>
 #include <linux/stddef.h>
+#include <linux/slab.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
@@ -81,8 +82,8 @@
 /*
  * virtual flags (16 and 32-bit versions)
  */
-#define VFLAGS	(*(unsigned short *)&(current->thread.v86flags))
-#define VEFLAGS	(current->thread.v86flags)
+#define VFLAGS	(*(unsigned short *)&(current->thread.vm86->v86flags))
+#define VEFLAGS	(current->thread.vm86->v86flags)
 
 #define set_flags(X, new, mask) \
 ((X) = ((X) & ~(mask)) | ((new) & (mask)))
@@ -130,6 +131,7 @@ struct pt_regs *save_v86_state(struct kernel_vm86_regs *regs)
 	struct tss_struct *tss;
 	struct pt_regs *ret;
 	unsigned long tmp;
+	struct kernel_vm86_info *vm86 = current->thread.vm86;
 
 	/*
 	 * This gets called from entry.S with interrupts disabled, but
@@ -138,29 +140,29 @@ struct pt_regs *save_v86_state(struct kernel_vm86_regs *regs)
 	 */
 	local_irq_enable();
 
-	if (!current->thread.vm86_info) {
+	if (!vm86 || !vm86->vm86_info) {
 		pr_alert("no vm86_info: BAD\n");
 		do_exit(SIGSEGV);
 	}
-	set_flags(regs->pt.flags, VEFLAGS, X86_EFLAGS_VIF | current->thread.v86mask);
-	tmp = copy_vm86_regs_to_user(&current->thread.vm86_info->regs, regs);
-	tmp += put_user(current->thread.screen_bitmap, &current->thread.vm86_info->screen_bitmap);
+	set_flags(regs->pt.flags, VEFLAGS, X86_EFLAGS_VIF | vm86->v86mask);
+	tmp = copy_vm86_regs_to_user(&vm86->vm86_info->regs, regs);
+	tmp += put_user(vm86->screen_bitmap, &vm86->vm86_info->screen_bitmap);
 	if (tmp) {
 		pr_alert("could not access userspace vm86_info\n");
 		do_exit(SIGSEGV);
 	}
 
 	tss = &per_cpu(cpu_tss, get_cpu());
-	current->thread.sp0 = current->thread.saved_sp0;
+	current->thread.sp0 = vm86->saved_sp0;
 	current->thread.sysenter_cs = __KERNEL_CS;
 	load_sp0(tss, &current->thread);
-	current->thread.saved_sp0 = 0;
+	vm86->saved_sp0 = 0;
 	put_cpu();
 
 	ret = KVM86->regs32;
 
-	ret->fs = current->thread.saved_fs;
-	set_user_gs(ret, current->thread.saved_gs);
+	ret->fs = vm86->saved_fs;
+	set_user_gs(ret, vm86->saved_gs);
 
 	return ret;
 }
@@ -249,8 +251,15 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 {
 	struct tss_struct *tss;
 	struct task_struct *tsk = current;
+	struct kernel_vm86_info *vm86 = tsk->thread.vm86;
 
-	if (tsk->thread.saved_sp0)
+	if (!vm86)
+	{
+		if (!(vm86 = kzalloc(sizeof(*vm86), GFP_KERNEL)))
+			return -ENOMEM;
+		tsk->thread.vm86 = vm86;
+	}
+	if (vm86->saved_sp0)
 		return -EPERM;
 	if (plus) {
 		if (copy_vm86_regs_from_user(&info->regs, &v86->regs,
@@ -266,7 +275,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 		memset(&info->vm86plus, 0, sizeof(struct vm86plus_info_struct));
 	}
 	info->regs32 = current_pt_regs();
-	tsk->thread.vm86_info = (struct vm86_struct __user *) v86;
+	vm86->vm86_info = (struct vm86_struct __user *) v86;
 
 /*
  * make sure the vm86() system call doesn't try to do anything silly
@@ -290,16 +299,16 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 
 	switch (info->cpu_type) {
 	case CPU_286:
-		tsk->thread.v86mask = 0;
+		vm86->v86mask = 0;
 		break;
 	case CPU_386:
-		tsk->thread.v86mask = X86_EFLAGS_NT | X86_EFLAGS_IOPL;
+		vm86->v86mask = X86_EFLAGS_NT | X86_EFLAGS_IOPL;
 		break;
 	case CPU_486:
-		tsk->thread.v86mask = X86_EFLAGS_AC | X86_EFLAGS_NT | X86_EFLAGS_IOPL;
+		vm86->v86mask = X86_EFLAGS_AC | X86_EFLAGS_NT | X86_EFLAGS_IOPL;
 		break;
 	default:
-		tsk->thread.v86mask = X86_EFLAGS_ID | X86_EFLAGS_AC | X86_EFLAGS_NT | X86_EFLAGS_IOPL;
+		vm86->v86mask = X86_EFLAGS_ID | X86_EFLAGS_AC | X86_EFLAGS_NT | X86_EFLAGS_IOPL;
 		break;
 	}
 
@@ -307,9 +316,9 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
  * Save old state, set default return value (%ax) to 0 (VM86_SIGNAL)
  */
 	info->regs32->ax = VM86_SIGNAL;
-	tsk->thread.saved_sp0 = tsk->thread.sp0;
-	tsk->thread.saved_fs = info->regs32->fs;
-	tsk->thread.saved_gs = get_user_gs(info->regs32);
+	vm86->saved_sp0 = tsk->thread.sp0;
+	vm86->saved_fs = info->regs32->fs;
+	vm86->saved_gs = get_user_gs(info->regs32);
 
 	tss = &per_cpu(cpu_tss, get_cpu());
 	tsk->thread.sp0 = (unsigned long) &info->VM86_TSS_ESP0;
@@ -318,7 +327,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 	load_sp0(tss, &tsk->thread);
 	put_cpu();
 
-	tsk->thread.screen_bitmap = info->screen_bitmap;
+	vm86->screen_bitmap = info->screen_bitmap;
 	if (info->flags & VM86_SCREEN_BITMAP)
 		mark_screen_rdonly(tsk->mm);
 
@@ -388,7 +397,7 @@ static inline void clear_AC(struct kernel_vm86_regs *regs)
 
 static inline void set_vflags_long(unsigned long flags, struct kernel_vm86_regs *regs)
 {
-	set_flags(VEFLAGS, flags, current->thread.v86mask);
+	set_flags(VEFLAGS, flags, current->thread.vm86->v86mask);
 	set_flags(regs->pt.flags, flags, SAFE_MASK);
 	if (flags & X86_EFLAGS_IF)
 		set_IF(regs);
@@ -398,7 +407,7 @@ static inline void set_vflags_long(unsigned long flags, struct kernel_vm86_regs
 
 static inline void set_vflags_short(unsigned short flags, struct kernel_vm86_regs *regs)
 {
-	set_flags(VFLAGS, flags, current->thread.v86mask);
+	set_flags(VFLAGS, flags, current->thread.vm86->v86mask);
 	set_flags(regs->pt.flags, flags, SAFE_MASK);
 	if (flags & X86_EFLAGS_IF)
 		set_IF(regs);
@@ -413,7 +422,7 @@ static inline unsigned long get_vflags(struct kernel_vm86_regs *regs)
 	if (VEFLAGS & X86_EFLAGS_VIF)
 		flags |= X86_EFLAGS_IF;
 	flags |= X86_EFLAGS_IOPL;
-	return flags | (VEFLAGS & current->thread.v86mask);
+	return flags | (VEFLAGS & current->thread.vm86->v86mask);
 }
 
 static inline int is_revectored(int nr, struct revectored_struct *bitmap)
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 81dcebf..5196ac4 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -315,12 +315,12 @@ check_v8086_mode(struct pt_regs *regs, unsigned long address,
 {
 	unsigned long bit;
 
-	if (!v8086_mode(regs))
+	if (!v8086_mode(regs) || !tsk->thread.vm86)
 		return;
 
 	bit = (address - 0xA0000) >> PAGE_SHIFT;
 	if (bit < 32)
-		tsk->thread.screen_bitmap |= 1 << bit;
+		tsk->thread.vm86->screen_bitmap |= 1 << bit;
 }
 
 static bool low_pfn(unsigned long pfn)
-- 
2.4.3

[PATCH 3/5] x86/vm86: Move fields from kernel_vm86_struct

[Brian Gerst]

Move the non-regs fields to the off-stack data.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/include/asm/vm86.h | 17 ++++++++--------
 arch/x86/kernel/vm86_32.c   | 48 ++++++++++++++++++++++-----------------------
 2 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/vm86.h b/arch/x86/include/asm/vm86.h
index 88f14e3..bc28a40 100644
--- a/arch/x86/include/asm/vm86.h
+++ b/arch/x86/include/asm/vm86.h
@@ -38,13 +38,7 @@ struct kernel_vm86_struct {
  * Therefore, pt_regs in fact points to a complete 'kernel_vm86_struct'
  * in kernelspace, hence we need not reget the data from userspace.
  */
-#define VM86_TSS_ESP0 flags
-	unsigned long flags;
-	unsigned long screen_bitmap;
-	unsigned long cpu_type;
-	struct revectored_struct int_revectored;
-	struct revectored_struct int21_revectored;
-	struct vm86plus_info_struct vm86plus;
+#define VM86_TSS_ESP0 regs32
 	struct pt_regs *regs32;   /* here we save the pointer to the old regs */
 /*
  * The below is not part of the structure, but the stack layout continues
@@ -60,12 +54,19 @@ struct kernel_vm86_struct {
 
 struct kernel_vm86_info {
 	struct vm86_struct __user *vm86_info;
-	unsigned long screen_bitmap;
 	unsigned long v86flags;
 	unsigned long v86mask;
 	unsigned long saved_sp0;
 	unsigned int saved_fs;
 	unsigned int saved_gs;
+
+	/* Must match vm86plus_struct after regs */
+	unsigned long flags;
+	unsigned long screen_bitmap;
+	unsigned long cpu_type;
+	struct revectored_struct int_revectored;
+	struct revectored_struct int21_revectored;
+	struct vm86plus_info_struct vm86plus;
 };
 
 #ifdef CONFIG_VM86
diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index 8b29c9b..f174602 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -68,7 +68,6 @@
 
 
 #define KVM86	((struct kernel_vm86_struct *)regs)
-#define VMPI	KVM86->vm86plus
 
 
 /*
@@ -111,18 +110,16 @@ static int copy_vm86_regs_to_user(struct vm86_regs __user *user,
 
 /* convert vm86_regs to kernel_vm86_regs */
 static int copy_vm86_regs_from_user(struct kernel_vm86_regs *regs,
-				    const struct vm86_regs __user *user,
-				    unsigned extra)
+				    const struct vm86_regs __user *user)
 {
 	int ret = 0;
 
 	/* copy ax-fs inclusive */
 	ret += copy_from_user(regs, user, offsetof(struct kernel_vm86_regs, pt.orig_ax));
-	/* copy orig_ax-__gsh+extra */
+	/* copy orig_ax-__gsh */
 	ret += copy_from_user(&regs->pt.orig_ax, &user->orig_eax,
 			      sizeof(struct kernel_vm86_regs) -
-			      offsetof(struct kernel_vm86_regs, pt.orig_ax) +
-			      extra);
+			      offsetof(struct kernel_vm86_regs, pt.orig_ax));
 	return ret;
 }
 
@@ -261,18 +258,20 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 	}
 	if (vm86->saved_sp0)
 		return -EPERM;
+	if (copy_vm86_regs_from_user(&info->regs, &v86->regs))
+		return -EFAULT;
 	if (plus) {
-		if (copy_vm86_regs_from_user(&info->regs, &v86->regs,
-			offsetof(struct kernel_vm86_struct, regs32) -
-			sizeof(info->regs)))
+		if (copy_from_user(&vm86->flags, &v86->flags,
+				   sizeof(struct vm86plus_struct) -
+				   offsetof(struct vm86plus_struct, flags)))
 			return -EFAULT;
-		info->vm86plus.is_vm86pus = 1;
+		vm86->vm86plus.is_vm86pus = 1;
 	} else {
-		if (copy_vm86_regs_from_user(&info->regs, &v86->regs,
-			offsetof(struct kernel_vm86_struct, vm86plus) -
-			sizeof(info->regs)))
+		if (copy_from_user(&vm86->flags, &v86->flags,
+				   sizeof(struct vm86_struct) -
+				   offsetof(struct vm86_struct, flags)))
 			return -EFAULT;
-		memset(&info->vm86plus, 0, sizeof(struct vm86plus_info_struct));
+		memset(&vm86->vm86plus, 0, sizeof(struct vm86plus_info_struct));
 	}
 	info->regs32 = current_pt_regs();
 	vm86->vm86_info = (struct vm86_struct __user *) v86;
@@ -297,7 +296,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 	info->regs.pt.flags |= info->regs32->flags & ~SAFE_MASK;
 	info->regs.pt.flags |= X86_VM_MASK;
 
-	switch (info->cpu_type) {
+	switch (vm86->cpu_type) {
 	case CPU_286:
 		vm86->v86mask = 0;
 		break;
@@ -327,8 +326,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 	load_sp0(tss, &tsk->thread);
 	put_cpu();
 
-	vm86->screen_bitmap = info->screen_bitmap;
-	if (info->flags & VM86_SCREEN_BITMAP)
+	if (vm86->flags & VM86_SCREEN_BITMAP)
 		mark_screen_rdonly(tsk->mm);
 
 	/*call __audit_syscall_exit since we do not exit via the normal paths */
@@ -520,12 +518,13 @@ static void do_int(struct kernel_vm86_regs *regs, int i,
 {
 	unsigned long __user *intr_ptr;
 	unsigned long segoffs;
+	struct kernel_vm86_info *vm86 = current->thread.vm86;
 
 	if (regs->pt.cs == BIOSSEG)
 		goto cannot_handle;
-	if (is_revectored(i, &KVM86->int_revectored))
+	if (is_revectored(i, &vm86->int_revectored))
 		goto cannot_handle;
-	if (i == 0x21 && is_revectored(AH(regs), &KVM86->int21_revectored))
+	if (i == 0x21 && is_revectored(AH(regs), &vm86->int21_revectored))
 		goto cannot_handle;
 	intr_ptr = (unsigned long __user *) (i << 2);
 	if (get_user(segoffs, intr_ptr))
@@ -549,7 +548,7 @@ cannot_handle:
 
 int handle_vm86_trap(struct kernel_vm86_regs *regs, long error_code, int trapno)
 {
-	if (VMPI.is_vm86pus) {
+	if (current->thread.vm86->vm86plus.is_vm86pus) {
 		if ((trapno == 3) || (trapno == 1)) {
 			KVM86->regs32->ax = VM86_TRAP + (trapno << 8);
 			/* setting this flag forces the code in entry_32.S to
@@ -576,12 +575,13 @@ void handle_vm86_fault(struct kernel_vm86_regs *regs, long error_code)
 	unsigned char __user *ssp;
 	unsigned short ip, sp, orig_flags;
 	int data32, pref_done;
+	struct vm86plus_info_struct *vmpi = &current->thread.vm86->vm86plus;
 
 #define CHECK_IF_IN_TRAP \
-	if (VMPI.vm86dbg_active && VMPI.vm86dbg_TFpendig) \
+	if (vmpi->vm86dbg_active && vmpi->vm86dbg_TFpendig) \
 		newflags |= X86_EFLAGS_TF
 #define VM86_FAULT_RETURN do { \
-	if (VMPI.force_return_for_pic  && (VEFLAGS & (X86_EFLAGS_IF | X86_EFLAGS_VIF))) \
+	if (vmpi->force_return_for_pic  && (VEFLAGS & (X86_EFLAGS_IF | X86_EFLAGS_VIF))) \
 		return_to_32bit(regs, VM86_PICRETURN); \
 	if (orig_flags & X86_EFLAGS_TF) \
 		handle_vm86_trap(regs, 0, 1); \
@@ -651,8 +651,8 @@ void handle_vm86_fault(struct kernel_vm86_regs *regs, long error_code)
 	case 0xcd: {
 		int intno = popb(csp, ip, simulate_sigsegv);
 		IP(regs) = ip;
-		if (VMPI.vm86dbg_active) {
-			if ((1 << (intno & 7)) & VMPI.vm86dbg_intxxtab[intno >> 3])
+		if (vmpi->vm86dbg_active) {
+			if ((1 << (intno & 7)) & vmpi->vm86dbg_intxxtab[intno >> 3])
 				return_to_32bit(regs, VM86_INTx + (intno << 8));
 		}
 		do_int(regs, intno, ssp, sp);
-- 
2.4.3

[PATCH 4/5] x86/vm86: Eliminate kernel_vm86_struct

[Brian Gerst]

Now there is no vm86-specific data left on the kernel stack while in
userspace, except for the 32-bit regs.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/include/asm/vm86.h | 25 +----------------
 arch/x86/kernel/vm86_32.c   | 68 +++++++++++++++++++--------------------------
 2 files changed, 29 insertions(+), 64 deletions(-)

diff --git a/arch/x86/include/asm/vm86.h b/arch/x86/include/asm/vm86.h
index bc28a40..84d4bda 100644
--- a/arch/x86/include/asm/vm86.h
+++ b/arch/x86/include/asm/vm86.h
@@ -28,32 +28,9 @@ struct kernel_vm86_regs {
 	unsigned short gs, __gsh;
 };
 
-struct kernel_vm86_struct {
-	struct kernel_vm86_regs regs;
-/*
- * the below part remains on the kernel stack while we are in VM86 mode.
- * 'tss.esp0' then contains the address of VM86_TSS_ESP0 below, and when we
- * get forced back from VM86, the CPU and "SAVE_ALL" will restore the above
- * 'struct kernel_vm86_regs' with the then actual values.
- * Therefore, pt_regs in fact points to a complete 'kernel_vm86_struct'
- * in kernelspace, hence we need not reget the data from userspace.
- */
-#define VM86_TSS_ESP0 regs32
-	struct pt_regs *regs32;   /* here we save the pointer to the old regs */
-/*
- * The below is not part of the structure, but the stack layout continues
- * this way. In front of 'return-eip' may be some data, depending on
- * compilation, so we don't rely on this and save the pointer to 'oldregs'
- * in 'regs32' above.
- * However, with GCC-2.7.2 and the current CFLAGS you see exactly this:
-
-	long return-eip;        from call to vm86()
-	struct pt_regs oldregs;  user space registers as saved by syscall
- */
-};
-
 struct kernel_vm86_info {
 	struct vm86_struct __user *vm86_info;
+	struct pt_regs *regs32;
 	unsigned long v86flags;
 	unsigned long v86mask;
 	unsigned long saved_sp0;
diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index f174602..d7aae93 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -67,9 +67,6 @@
  */
 
 
-#define KVM86	((struct kernel_vm86_struct *)regs)
-
-
 /*
  * 8- and 16-bit register defines..
  */
@@ -156,7 +153,7 @@ struct pt_regs *save_v86_state(struct kernel_vm86_regs *regs)
 	vm86->saved_sp0 = 0;
 	put_cpu();
 
-	ret = KVM86->regs32;
+	ret = vm86->regs32;
 
 	ret->fs = vm86->saved_fs;
 	set_user_gs(ret, vm86->saved_gs);
@@ -199,29 +196,16 @@ out:
 
 
 static int do_vm86_irq_handling(int subfunction, int irqnumber);
-static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
-			struct kernel_vm86_struct *info);
+static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus);
 
 SYSCALL_DEFINE1(vm86old, struct vm86_struct __user *, v86)
 {
-	struct kernel_vm86_struct info; /* declare this _on top_,
-					 * this avoids wasting of stack space.
-					 * This remains on the stack until we
-					 * return to 32 bit user space.
-					 */
-
-	return do_sys_vm86((struct vm86plus_struct __user *) v86, false, &info);
+	return do_sys_vm86((struct vm86plus_struct __user *) v86, false);
 }
 
 
 SYSCALL_DEFINE2(vm86, unsigned long, cmd, unsigned long, arg)
 {
-	struct kernel_vm86_struct info; /* declare this _on top_,
-					 * this avoids wasting of stack space.
-					 * This remains on the stack until we
-					 * return to 32 bit user space.
-					 */
-
 	switch (cmd) {
 	case VM86_REQUEST_IRQ:
 	case VM86_FREE_IRQ:
@@ -239,16 +223,17 @@ SYSCALL_DEFINE2(vm86, unsigned long, cmd, unsigned long, arg)
 	}
 
 	/* we come here only for functions VM86_ENTER, VM86_ENTER_NO_BYPASS */
-	return do_sys_vm86((struct vm86plus_struct __user *) arg, true, &info);
+	return do_sys_vm86((struct vm86plus_struct __user *) arg, true);
 }
 
 
-static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
-			struct kernel_vm86_struct *info)
+static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus)
 {
 	struct tss_struct *tss;
 	struct task_struct *tsk = current;
 	struct kernel_vm86_info *vm86 = tsk->thread.vm86;
+	struct kernel_vm86_regs regs;
+	struct pt_regs *regs32 = current_pt_regs();
 
 	if (!vm86)
 	{
@@ -258,7 +243,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 	}
 	if (vm86->saved_sp0)
 		return -EPERM;
-	if (copy_vm86_regs_from_user(&info->regs, &v86->regs))
+	if (copy_vm86_regs_from_user(&regs, &v86->regs))
 		return -EFAULT;
 	if (plus) {
 		if (copy_from_user(&vm86->flags, &v86->flags,
@@ -273,17 +258,17 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 			return -EFAULT;
 		memset(&vm86->vm86plus, 0, sizeof(struct vm86plus_info_struct));
 	}
-	info->regs32 = current_pt_regs();
+	vm86->regs32 = regs32;
 	vm86->vm86_info = (struct vm86_struct __user *) v86;
 
 /*
  * make sure the vm86() system call doesn't try to do anything silly
  */
-	info->regs.pt.ds = 0;
-	info->regs.pt.es = 0;
-	info->regs.pt.fs = 0;
+	regs.pt.ds = 0;
+	regs.pt.es = 0;
+	regs.pt.fs = 0;
 #ifndef CONFIG_X86_32_LAZY_GS
-	info->regs.pt.gs = 0;
+	regs.pt.gs = 0;
 #endif
 
 /*
@@ -291,10 +276,10 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
  * has set it up safely, so this makes sure interrupt etc flags are
  * inherited from protected mode.
  */
-	VEFLAGS = info->regs.pt.flags;
-	info->regs.pt.flags &= SAFE_MASK;
-	info->regs.pt.flags |= info->regs32->flags & ~SAFE_MASK;
-	info->regs.pt.flags |= X86_VM_MASK;
+	VEFLAGS = regs.pt.flags;
+	regs.pt.flags &= SAFE_MASK;
+	regs.pt.flags |= regs32->flags & ~SAFE_MASK;
+	regs.pt.flags |= X86_VM_MASK;
 
 	switch (vm86->cpu_type) {
 	case CPU_286:
@@ -314,13 +299,14 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 /*
  * Save old state, set default return value (%ax) to 0 (VM86_SIGNAL)
  */
-	info->regs32->ax = VM86_SIGNAL;
+	regs32->ax = VM86_SIGNAL;
 	vm86->saved_sp0 = tsk->thread.sp0;
-	vm86->saved_fs = info->regs32->fs;
-	vm86->saved_gs = get_user_gs(info->regs32);
+	vm86->saved_fs = regs32->fs;
+	vm86->saved_gs = get_user_gs(regs32);
 
 	tss = &per_cpu(cpu_tss, get_cpu());
-	tsk->thread.sp0 = (unsigned long) &info->VM86_TSS_ESP0;
+	/* Set new sp0 right below 32-bit regs */
+	tsk->thread.sp0 = (unsigned long) regs32;
 	if (cpu_has_sep)
 		tsk->thread.sysenter_cs = 0;
 	load_sp0(tss, &tsk->thread);
@@ -343,7 +329,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 #endif
 		"jmp resume_userspace"
 		: /* no outputs */
-		:"r" (&info->regs), "r" (task_thread_info(tsk)), "r" (0));
+		:"r" (&regs), "r" (task_thread_info(tsk)), "r" (0));
 	return 0;	/* we never return here */
 }
 
@@ -548,12 +534,14 @@ cannot_handle:
 
 int handle_vm86_trap(struct kernel_vm86_regs *regs, long error_code, int trapno)
 {
-	if (current->thread.vm86->vm86plus.is_vm86pus) {
+	struct kernel_vm86_info *vm86 = current->thread.vm86;
+
+	if (vm86->vm86plus.is_vm86pus) {
 		if ((trapno == 3) || (trapno == 1)) {
-			KVM86->regs32->ax = VM86_TRAP + (trapno << 8);
+			vm86->regs32->ax = VM86_TRAP + (trapno << 8);
 			/* setting this flag forces the code in entry_32.S to
 			   the path where we call save_v86_state() and change
-			   the stack pointer to KVM86->regs32 */
+			   the stack pointer to regs32 */
 			set_thread_flag(TIF_NOTIFY_RESUME);
 			return 0;
 		}
-- 
2.4.3

[PATCH 5/5] x86/vm86: Use the normal pt_regs area for vm86

[Brian Gerst]

Change to use the normal pt_regs area to enter and exit vm86 mode.  This is
done by increasing the padding at the top of the stack to make room for the
extra vm86 segment slots in the IRET frame.  It then saves the 32-bit regs
in the off-stack vm86 data, and copies in the vm86 regs.  Exiting back to
32-bit mode does the reverse.  This allows removing the hacks to jump directly
into the exit asm code due to having to change the stack pointer.  Returning
normally from the vm86 syscall and the exception handlers allows things like
ptrace and auditing to work properly.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/entry/entry_32.S          |  24 +------
 arch/x86/include/asm/thread_info.h |  11 ++--
 arch/x86/include/asm/vm86.h        |   6 +-
 arch/x86/kernel/signal.c           |   3 +
 arch/x86/kernel/vm86_32.c          | 129 ++++++++++++++++---------------------
 5 files changed, 69 insertions(+), 104 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 21dc60a..f940e24 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -525,34 +525,12 @@ work_resched:
 
 work_notifysig:					# deal with pending signals and
 						# notify-resume requests
-#ifdef CONFIG_VM86
-	testl	$X86_EFLAGS_VM, PT_EFLAGS(%esp)
-	movl	%esp, %eax
-	jnz	work_notifysig_v86		# returning to kernel-space or
-						# vm86-space
-1:
-#else
-	movl	%esp, %eax
-#endif
 	TRACE_IRQS_ON
 	ENABLE_INTERRUPTS(CLBR_NONE)
-	movb	PT_CS(%esp), %bl
-	andb	$SEGMENT_RPL_MASK, %bl
-	cmpb	$USER_RPL, %bl
-	jb	resume_kernel
+	movl	%esp, %eax
 	xorl	%edx, %edx
 	call	do_notify_resume
 	jmp	resume_userspace
-
-#ifdef CONFIG_VM86
-	ALIGN
-work_notifysig_v86:
-	pushl	%ecx				# save ti_flags for do_notify_resume
-	call	save_v86_state			# %eax contains pt_regs pointer
-	popl	%ecx
-	movl	%eax, %esp
-	jmp	1b
-#endif
 END(work_pending)
 
 	# perform syscall exit tracing
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 225ee54..fdad5c2 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -27,14 +27,17 @@
  * Without this offset, that can result in a page fault.  (We are
  * careful that, in this case, the value we read doesn't matter.)
  *
- * In vm86 mode, the hardware frame is much longer still, but we neither
- * access the extra members from NMI context, nor do we write such a
- * frame at sp0 at all.
+ * In vm86 mode, the hardware frame is much longer still, so add 16
+ * bytes to make room for the real-mode segments.
  *
  * x86_64 has a fixed-length stack frame.
  */
 #ifdef CONFIG_X86_32
-# define TOP_OF_KERNEL_STACK_PADDING 8
+# ifdef CONFIG_VM86
+#  define TOP_OF_KERNEL_STACK_PADDING 16
+# else
+#  define TOP_OF_KERNEL_STACK_PADDING 8
+# endif
 #else
 # define TOP_OF_KERNEL_STACK_PADDING 0
 #endif
diff --git a/arch/x86/include/asm/vm86.h b/arch/x86/include/asm/vm86.h
index 84d4bda..a72a1a2 100644
--- a/arch/x86/include/asm/vm86.h
+++ b/arch/x86/include/asm/vm86.h
@@ -30,7 +30,7 @@ struct kernel_vm86_regs {
 
 struct kernel_vm86_info {
 	struct vm86_struct __user *vm86_info;
-	struct pt_regs *regs32;
+	struct pt_regs regs32;
 	unsigned long v86flags;
 	unsigned long v86mask;
 	unsigned long saved_sp0;
@@ -50,7 +50,7 @@ struct kernel_vm86_info {
 
 void handle_vm86_fault(struct kernel_vm86_regs *, long);
 int handle_vm86_trap(struct kernel_vm86_regs *, long, int);
-struct pt_regs *save_v86_state(struct kernel_vm86_regs *);
+void save_v86_state(struct kernel_vm86_regs *, int);
 
 struct task_struct;
 void release_vm86_irqs(struct task_struct *);
@@ -65,6 +65,8 @@ static inline int handle_vm86_trap(struct kernel_vm86_regs *a, long b, int c)
 	return 0;
 }
 
+static inline void save_v86_state(struct kernel_vm86_regs *, int) { }
+
 #endif /* CONFIG_VM86 */
 
 #endif /* _ASM_X86_VM86_H */
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 7e88cc7..bfd736e 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -635,6 +635,9 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	bool stepping, failed;
 	struct fpu *fpu = &current->thread.fpu;
 
+	if (v8086_mode(regs))
+		save_v86_state((struct kernel_vm86_regs *) regs, VM86_SIGNAL);
+
 	/* Are we from a system call? */
 	if (syscall_get_nr(current, regs) >= 0) {
 		/* If so, check system call restarting.. */
diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index d7aae93..bde9d2e 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -50,6 +50,7 @@
 #include <asm/io.h>
 #include <asm/tlbflush.h>
 #include <asm/irq.h>
+#include <asm/traps.h>
 
 /*
  * Known problems:
@@ -120,10 +121,9 @@ static int copy_vm86_regs_from_user(struct kernel_vm86_regs *regs,
 	return ret;
 }
 
-struct pt_regs *save_v86_state(struct kernel_vm86_regs *regs)
+void save_v86_state(struct kernel_vm86_regs *regs, int retval)
 {
 	struct tss_struct *tss;
-	struct pt_regs *ret;
 	unsigned long tmp;
 	struct kernel_vm86_info *vm86 = current->thread.vm86;
 
@@ -153,12 +153,12 @@ struct pt_regs *save_v86_state(struct kernel_vm86_regs *regs)
 	vm86->saved_sp0 = 0;
 	put_cpu();
 
-	ret = vm86->regs32;
+	memcpy(&regs->pt, &vm86->regs32, sizeof(struct pt_regs));
 
-	ret->fs = vm86->saved_fs;
-	set_user_gs(ret, vm86->saved_gs);
+	regs->pt.fs = vm86->saved_fs;
+	set_user_gs(&regs->pt, vm86->saved_gs);
 
-	return ret;
+	regs->pt.ax = retval;
 }
 
 static void mark_screen_rdonly(struct mm_struct *mm)
@@ -232,8 +232,8 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus)
 	struct tss_struct *tss;
 	struct task_struct *tsk = current;
 	struct kernel_vm86_info *vm86 = tsk->thread.vm86;
-	struct kernel_vm86_regs regs;
-	struct pt_regs *regs32 = current_pt_regs();
+	struct kernel_vm86_regs vm86regs;
+	struct pt_regs *regs = current_pt_regs();
 
 	if (!vm86)
 	{
@@ -243,7 +243,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus)
 	}
 	if (vm86->saved_sp0)
 		return -EPERM;
-	if (copy_vm86_regs_from_user(&regs, &v86->regs))
+	if (copy_vm86_regs_from_user(&vm86regs, &v86->regs))
 		return -EFAULT;
 	if (plus) {
 		if (copy_from_user(&vm86->flags, &v86->flags,
@@ -258,17 +258,17 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus)
 			return -EFAULT;
 		memset(&vm86->vm86plus, 0, sizeof(struct vm86plus_info_struct));
 	}
-	vm86->regs32 = regs32;
+	memcpy(&vm86->regs32, regs, sizeof(struct pt_regs));
 	vm86->vm86_info = (struct vm86_struct __user *) v86;
 
 /*
  * make sure the vm86() system call doesn't try to do anything silly
  */
-	regs.pt.ds = 0;
-	regs.pt.es = 0;
-	regs.pt.fs = 0;
+	vm86regs.pt.ds = 0;
+	vm86regs.pt.es = 0;
+	vm86regs.pt.fs = 0;
 #ifndef CONFIG_X86_32_LAZY_GS
-	regs.pt.gs = 0;
+	vm86regs.pt.gs = 0;
 #endif
 
 /*
@@ -276,10 +276,10 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus)
  * has set it up safely, so this makes sure interrupt etc flags are
  * inherited from protected mode.
  */
-	VEFLAGS = regs.pt.flags;
-	regs.pt.flags &= SAFE_MASK;
-	regs.pt.flags |= regs32->flags & ~SAFE_MASK;
-	regs.pt.flags |= X86_VM_MASK;
+	VEFLAGS = vm86regs.pt.flags;
+	vm86regs.pt.flags &= SAFE_MASK;
+	vm86regs.pt.flags |= vm86->regs32.flags & ~SAFE_MASK;
+	vm86regs.pt.flags |= X86_VM_MASK;
 
 	switch (vm86->cpu_type) {
 	case CPU_286:
@@ -297,16 +297,15 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus)
 	}
 
 /*
- * Save old state, set default return value (%ax) to 0 (VM86_SIGNAL)
+ * Save old state
  */
-	regs32->ax = VM86_SIGNAL;
 	vm86->saved_sp0 = tsk->thread.sp0;
-	vm86->saved_fs = regs32->fs;
-	vm86->saved_gs = get_user_gs(regs32);
+	vm86->saved_fs = vm86->regs32.fs;
+	vm86->saved_gs = get_user_gs(&vm86->regs32);
 
 	tss = &per_cpu(cpu_tss, get_cpu());
-	/* Set new sp0 right below 32-bit regs */
-	tsk->thread.sp0 = (unsigned long) regs32;
+	/* make room for real-mode segments */
+	tsk->thread.sp0 += 16;
 	if (cpu_has_sep)
 		tsk->thread.sysenter_cs = 0;
 	load_sp0(tss, &tsk->thread);
@@ -315,41 +314,14 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus)
 	if (vm86->flags & VM86_SCREEN_BITMAP)
 		mark_screen_rdonly(tsk->mm);
 
-	/*call __audit_syscall_exit since we do not exit via the normal paths */
-#ifdef CONFIG_AUDITSYSCALL
-	if (unlikely(current->audit_context))
-		__audit_syscall_exit(1, 0);
-#endif
-
-	__asm__ __volatile__(
-		"movl %0,%%esp\n\t"
-		"movl %1,%%ebp\n\t"
-#ifdef CONFIG_X86_32_LAZY_GS
-		"mov  %2, %%gs\n\t"
-#endif
-		"jmp resume_userspace"
-		: /* no outputs */
-		:"r" (&regs), "r" (task_thread_info(tsk)), "r" (0));
-	return 0;	/* we never return here */
-}
-
-static inline void return_to_32bit(struct kernel_vm86_regs *regs16, int retval)
-{
-	struct pt_regs *regs32;
-
-	regs32 = save_v86_state(regs16);
-	regs32->ax = retval;
-	__asm__ __volatile__("movl %0,%%esp\n\t"
-		"movl %1,%%ebp\n\t"
-		"jmp resume_userspace"
-		: : "r" (regs32), "r" (current_thread_info()));
+	memcpy((struct kernel_vm86_regs *)regs, &vm86regs, sizeof(vm86regs));
+	force_iret();
+	return regs->ax;
 }
 
 static inline void set_IF(struct kernel_vm86_regs *regs)
 {
 	VEFLAGS |= X86_EFLAGS_VIF;
-	if (VEFLAGS & X86_EFLAGS_VIP)
-		return_to_32bit(regs, VM86_STI);
 }
 
 static inline void clear_IF(struct kernel_vm86_regs *regs)
@@ -529,7 +501,7 @@ static void do_int(struct kernel_vm86_regs *regs, int i,
 	return;
 
 cannot_handle:
-	return_to_32bit(regs, VM86_INTx + (i << 8));
+	save_v86_state(regs, VM86_INTx + (i << 8));
 }
 
 int handle_vm86_trap(struct kernel_vm86_regs *regs, long error_code, int trapno)
@@ -538,11 +510,7 @@ int handle_vm86_trap(struct kernel_vm86_regs *regs, long error_code, int trapno)
 
 	if (vm86->vm86plus.is_vm86pus) {
 		if ((trapno == 3) || (trapno == 1)) {
-			vm86->regs32->ax = VM86_TRAP + (trapno << 8);
-			/* setting this flag forces the code in entry_32.S to
-			   the path where we call save_v86_state() and change
-			   the stack pointer to regs32 */
-			set_thread_flag(TIF_NOTIFY_RESUME);
+			save_v86_state(regs, VM86_TRAP + (trapno << 8));
 			return 0;
 		}
 		do_int(regs, trapno, (unsigned char __user *) (regs->pt.ss << 4), SP(regs));
@@ -568,12 +536,6 @@ void handle_vm86_fault(struct kernel_vm86_regs *regs, long error_code)
 #define CHECK_IF_IN_TRAP \
 	if (vmpi->vm86dbg_active && vmpi->vm86dbg_TFpendig) \
 		newflags |= X86_EFLAGS_TF
-#define VM86_FAULT_RETURN do { \
-	if (vmpi->force_return_for_pic  && (VEFLAGS & (X86_EFLAGS_IF | X86_EFLAGS_VIF))) \
-		return_to_32bit(regs, VM86_PICRETURN); \
-	if (orig_flags & X86_EFLAGS_TF) \
-		handle_vm86_trap(regs, 0, 1); \
-	return; } while (0)
 
 	orig_flags = *(unsigned short *)&regs->pt.flags;
 
@@ -612,7 +574,7 @@ void handle_vm86_fault(struct kernel_vm86_regs *regs, long error_code)
 			SP(regs) -= 2;
 		}
 		IP(regs) = ip;
-		VM86_FAULT_RETURN;
+		goto vm86_fault_return;
 
 	/* popf */
 	case 0x9d:
@@ -632,7 +594,7 @@ void handle_vm86_fault(struct kernel_vm86_regs *regs, long error_code)
 		else
 			set_vflags_short(newflags, regs);
 
-		VM86_FAULT_RETURN;
+		goto check_vip;
 		}
 
 	/* int xx */
@@ -640,8 +602,10 @@ void handle_vm86_fault(struct kernel_vm86_regs *regs, long error_code)
 		int intno = popb(csp, ip, simulate_sigsegv);
 		IP(regs) = ip;
 		if (vmpi->vm86dbg_active) {
-			if ((1 << (intno & 7)) & vmpi->vm86dbg_intxxtab[intno >> 3])
-				return_to_32bit(regs, VM86_INTx + (intno << 8));
+			if ((1 << (intno & 7)) & vmpi->vm86dbg_intxxtab[intno >> 3]) {
+				save_v86_state(regs, VM86_INTx + (intno << 8));
+				return;
+			}
 		}
 		do_int(regs, intno, ssp, sp);
 		return;
@@ -672,14 +636,14 @@ void handle_vm86_fault(struct kernel_vm86_regs *regs, long error_code)
 		} else {
 			set_vflags_short(newflags, regs);
 		}
-		VM86_FAULT_RETURN;
+		goto check_vip;
 		}
 
 	/* cli */
 	case 0xfa:
 		IP(regs) = ip;
 		clear_IF(regs);
-		VM86_FAULT_RETURN;
+		goto vm86_fault_return;
 
 	/* sti */
 	/*
@@ -691,14 +655,29 @@ void handle_vm86_fault(struct kernel_vm86_regs *regs, long error_code)
 	case 0xfb:
 		IP(regs) = ip;
 		set_IF(regs);
-		VM86_FAULT_RETURN;
+		goto check_vip;
 
 	default:
-		return_to_32bit(regs, VM86_UNKNOWN);
+		save_v86_state(regs, VM86_UNKNOWN);
 	}
 
 	return;
 
+check_vip:
+	if (VEFLAGS & X86_EFLAGS_VIP) {
+		save_v86_state(regs, VM86_STI);
+		return;
+	}
+
+vm86_fault_return:
+	if (vmpi->force_return_for_pic  && (VEFLAGS & (X86_EFLAGS_IF | X86_EFLAGS_VIF))) {
+		save_v86_state(regs, VM86_PICRETURN);
+		return;
+	}
+	if (orig_flags & X86_EFLAGS_TF)
+		handle_vm86_trap(regs, 0, X86_TRAP_DB);
+	return;
+
 simulate_sigsegv:
 	/* FIXME: After a long discussion with Stas we finally
 	 *        agreed, that this is wrong. Here we should
@@ -710,7 +689,7 @@ simulate_sigsegv:
 	 *        should be a mixture of the two, but how do we
 	 *        get the information? [KD]
 	 */
-	return_to_32bit(regs, VM86_UNKNOWN);
+	save_v86_state(regs, VM86_UNKNOWN);
 }
 
 /* ---------------- vm86 special IRQ passing stuff ----------------- */
-- 
2.4.3

Re: [PATCH 1/5] x86/vm86: Move userspace accesses to do_sys_vm86()

[Andy Lutomirski]

On Fri, Jul 10, 2015 at 10:09 PM, Brian Gerst <brgerst@gmail.com> wrote:
> Move the userspace accesses down into the common function in
> preparation for the next set of patches.
>

One thing I don't like about the current code that makes these patches
harder to review is the bizarre approach to copying.  If you changed
this:

> -       tmp = copy_vm86_regs_from_user(&info.regs, &v86->regs,
> -                                      offsetof(struct kernel_vm86_struct, vm86plus) -
> -                                      sizeof(info.regs));

into a normal field-by-field get_user / copy_from_user (the latter for
the big regs struct) then it would be clear what the ABI is and it
would be much easier to read the patches and confirm that you aren't
accidentally changing the ABI.

You could also get rid of the constraint that certain fields in
apparently kernel-internal structs had to be in a certain order.

Other than that, patches 1-4 look good on cursory inspection.  I'll
look more carefully later.  I need to think about patch 5 more.

--Andy

Re: [PATCH 1/5] x86/vm86: Move userspace accesses to do_sys_vm86()

[Brian Gerst]

On Sat, Jul 11, 2015 at 12:37 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Fri, Jul 10, 2015 at 10:09 PM, Brian Gerst <brgerst@gmail.com> wrote:
>> Move the userspace accesses down into the common function in
>> preparation for the next set of patches.
>>
>
> One thing I don't like about the current code that makes these patches
> harder to review is the bizarre approach to copying.  If you changed
> this:
>
>> -       tmp = copy_vm86_regs_from_user(&info.regs, &v86->regs,
>> -                                      offsetof(struct kernel_vm86_struct, vm86plus) -
>> -                                      sizeof(info.regs));
>
> into a normal field-by-field get_user / copy_from_user (the latter for
> the big regs struct) then it would be clear what the ABI is and it
> would be much easier to read the patches and confirm that you aren't
> accidentally changing the ABI.
>
> You could also get rid of the constraint that certain fields in
> apparently kernel-internal structs had to be in a certain order.
>
> Other than that, patches 1-4 look good on cursory inspection.  I'll
> look more carefully later.  I need to think about patch 5 more.
>
> --Andy

Any other comments before I start working on v2?

--
Brian Gerst

Re: [PATCH 1/5] x86/vm86: Move userspace accesses to do_sys_vm86()

[Andy Lutomirski]

On Mon, Jul 13, 2015 at 9:45 AM, Brian Gerst <brgerst@gmail.com> wrote:
> On Sat, Jul 11, 2015 at 12:37 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Fri, Jul 10, 2015 at 10:09 PM, Brian Gerst <brgerst@gmail.com> wrote:
>>> Move the userspace accesses down into the common function in
>>> preparation for the next set of patches.
>>>
>>
>> One thing I don't like about the current code that makes these patches
>> harder to review is the bizarre approach to copying.  If you changed
>> this:
>>
>>> -       tmp = copy_vm86_regs_from_user(&info.regs, &v86->regs,
>>> -                                      offsetof(struct kernel_vm86_struct, vm86plus) -
>>> -                                      sizeof(info.regs));
>>
>> into a normal field-by-field get_user / copy_from_user (the latter for
>> the big regs struct) then it would be clear what the ABI is and it
>> would be much easier to read the patches and confirm that you aren't
>> accidentally changing the ABI.
>>
>> You could also get rid of the constraint that certain fields in
>> apparently kernel-internal structs had to be in a certain order.
>>
>> Other than that, patches 1-4 look good on cursory inspection.  I'll
>> look more carefully later.  I need to think about patch 5 more.
>>
>> --Andy
>
> Any other comments before I start working on v2?
>

Nothing major.  I'm a bit nervous about leaving ds, es, fs, and gs in
pt_regs more or less undefined until save_v86_state happens, but it's
unlikely that there's any ABI to break there.  The results from perf
might be a bit odd with your patches applied.  Of course, they're
probably useless without your patch.

It might also be worth renaming save_v86_state in patch 5.

Do your patches pass my upgraded entry_from_vm86 test?  You're
changing handle_vm86_trap so it always returns, which may have
unexpected side effects (or I missed something in your patch).

--Andy

Re: [PATCH 1/5] x86/vm86: Move userspace accesses to do_sys_vm86()

[Brian Gerst]

On Mon, Jul 13, 2015 at 2:36 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Jul 13, 2015 at 9:45 AM, Brian Gerst <brgerst@gmail.com> wrote:
>> On Sat, Jul 11, 2015 at 12:37 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Fri, Jul 10, 2015 at 10:09 PM, Brian Gerst <brgerst@gmail.com> wrote:
>>>> Move the userspace accesses down into the common function in
>>>> preparation for the next set of patches.
>>>>
>>>
>>> One thing I don't like about the current code that makes these patches
>>> harder to review is the bizarre approach to copying.  If you changed
>>> this:
>>>
>>>> -       tmp = copy_vm86_regs_from_user(&info.regs, &v86->regs,
>>>> -                                      offsetof(struct kernel_vm86_struct, vm86plus) -
>>>> -                                      sizeof(info.regs));
>>>
>>> into a normal field-by-field get_user / copy_from_user (the latter for
>>> the big regs struct) then it would be clear what the ABI is and it
>>> would be much easier to read the patches and confirm that you aren't
>>> accidentally changing the ABI.
>>>
>>> You could also get rid of the constraint that certain fields in
>>> apparently kernel-internal structs had to be in a certain order.
>>>
>>> Other than that, patches 1-4 look good on cursory inspection.  I'll
>>> look more carefully later.  I need to think about patch 5 more.
>>>
>>> --Andy
>>
>> Any other comments before I start working on v2?
>>
>
> Nothing major.  I'm a bit nervous about leaving ds, es, fs, and gs in
> pt_regs more or less undefined until save_v86_state happens, but it's
> unlikely that there's any ABI to break there.

They are not undefined.  The CPU sets them to NULL on exit from vm86
mode, after pushing them before the normal exception frame.  The entry
code then pushes these NULL values into the normal segment slots in
pt_regs.  The NULL values would be visible to ptrace (which should be
taught to look at the alternate slots), but I don't see any other
problems.

> The results from perf
> might be a bit odd with your patches applied.  Of course, they're
> probably useless without your patch.
>
> It might also be worth renaming save_v86_state in patch 5.
>
> Do your patches pass my upgraded entry_from_vm86 test?  You're
> changing handle_vm86_trap so it always returns, which may have
> unexpected side effects (or I missed something in your patch).

Yes, it passes all the tests.  Changing the trap/fault handlers to
always return (instead of switching the stack pointer and jumping to
the exit asm) is the whole point of this patch set.  save_v86_state()
now just swaps the register state in place and sets the syscall return
value.  This allows returning from the exception handler in the normal
manner (skipping the signal if the exception was handled).  An
unhandled exception like #UD will raise a signal, which then is
handled by the change in handle_signal(), as well as external signals.

--
Brian Gerst