[PATCH v2 1/1] x86/vm86: fix vm86 struct leak on copy_from_user() failure

[fffsqian]

From: Qingshuang Fu <fuqingshuang@xxxxxxxxxx>

When copy_from_user() fails during vm86 setup in do_sys_vm86(),
the newly allocated vm86 structure is not freed, leading to a
memory leak that can be exploited by users with appropriate
privileges to cause DoS through resource exhaustion.

The leak occurs in multiple error paths after kzalloc_obj()
successfully allocates the vm86 structure and assigns it to
tsk->thread.vm86. Subsequent copy_from_user() failures for
vm86_struct, int_revectored, int21_revectored, or vm86plus data
leave the allocated memory orphaned.

Fix this by saving the original vm86 pointer before allocation.
On error cleanup, if a new vm86 was allocated this call, restore
the original pointer and free the new allocation.

Security impact: This memory leak can lead to system DoS via OOM if
exploited by unprivileged local users, provided the system has
mmap_min_addr set to 0. The regression was introduced in commit
9fda6a0681e0 ("x86/vm86: Move vm86 fields out of 'thread_struct'") back
in 2015, and affects all 32-bit x86 kernels built with CONFIG_VM86
enabled.

Fixes: 9fda6a0681e0 ("x86/vm86: Move vm86 fields out of 'thread_struct'")
Signed-off-by: Qingshuang Fu <fuqingshuang@xxxxxxxxxx>
---
 arch/x86/kernel/vm86_32.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index b4c1cabc7a4b..538066eaa9a4 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -200,6 +200,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
 {
 	struct task_struct *tsk = current;
 	struct vm86 *vm86 = tsk->thread.vm86;
+	struct vm86 *old_vm86 = vm86;
 	struct kernel_vm86_regs vm86regs;
 	struct pt_regs *regs = current_pt_regs();
 	unsigned long err = 0;
@@ -240,15 +241,18 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
 		return -EPERM;
 
 	if (copy_from_user(&v, user_vm86,
-			offsetof(struct vm86_struct, int_revectored)))
-		return -EFAULT;
+			offsetof(struct vm86_struct, int_revectored))) {
+		err = -EFAULT;
+		goto cleanup;
+	}
 
 
 	/* VM86_SCREEN_BITMAP had numerous bugs and appears to have no users. */
 	if (v.flags & VM86_SCREEN_BITMAP) {
 		pr_info_once("vm86: '%s' uses VM86_SCREEN_BITMAP, which is no longer supported\n",
 			     current->comm);
-		return -EINVAL;
+		err = -EINVAL;
+		goto cleanup;
 	}
 
 	memset(&vm86regs, 0, sizeof(vm86regs));
@@ -275,16 +279,22 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
 
 	if (copy_from_user(&vm86->int_revectored,
 			   &user_vm86->int_revectored,
-			   sizeof(struct revectored_struct)))
-		return -EFAULT;
+			   sizeof(struct revectored_struct))) {
+		err = -EFAULT;
+		goto cleanup;
+	}
 	if (copy_from_user(&vm86->int21_revectored,
 			   &user_vm86->int21_revectored,
-			   sizeof(struct revectored_struct)))
-		return -EFAULT;
+			   sizeof(struct revectored_struct))) {
+		err = -EFAULT;
+		goto cleanup;
+	}
 	if (plus) {
 		if (copy_from_user(&vm86->vm86plus, &user_vm86->vm86plus,
-				   sizeof(struct vm86plus_info_struct)))
-			return -EFAULT;
+				   sizeof(struct vm86plus_info_struct))) {
+			err = -EFAULT;
+			goto cleanup;
+		}
 		vm86->vm86plus.is_vm86pus = 1;
 	} else
 		memset(&vm86->vm86plus, 0,
@@ -340,6 +350,13 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
 
 	memcpy((struct kernel_vm86_regs *)regs, &vm86regs, sizeof(vm86regs));
 	return regs->ax;
+
+cleanup:
+	if (vm86 != old_vm86) {
+		kfree(vm86);
+		tsk->thread.vm86 = old_vm86;
+	}
+	return err;
 }
 
 static inline void set_IF(struct kernel_vm86_regs *regs)

base-commit: ab9de95c9cf952332ab79453b4b5d1bfca8e514f
-- 
2.25.1