linux-kernel - [PATCH] prctl: fix overwrite last but one entry in auxv vector

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <YFeuWsnb6qKEU8+h@grain>
Date:   Sun, 21 Mar 2021 23:36:42 +0300
From:   Cyrill Gorcunov <gorcunov@...il.com>
To:     LKML <linux-kernel@...r.kernel.org>
Cc:     Alexey Dobriyan <adobriyan@...il.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Andrey Vagin <avagin@...il.com>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: [PATCH] prctl: fix overwrite last but one entry in auxv vector

Alexey reported that current PR_SET_MM_AUXV (and PR_SET_MM_MAP) overwrite
too many entries on non 64bit kernels. This is because auxv is defined
as an array of longs and in result access to AT_VECTOR_SIZE - 2 entry
is not a type of auxv entry but rather an entry before the last one.

Since it is a common code for all architectures lets use __BITS_PER_LONG
definition to determinate each type/value pair in auxv_t is fitting
into `long` or not.

Note that on compat mode (ie Elf32 running in 64bit compiled kernel)
the preallocated vector size will be big enough to carry all entries
and zapping two entries at the end of the vector won't cause problems.

Same time lets drop useless task_lock()/task_unlock() calls from
PR_SET_MM_AUXV. It doesn't protect anything here and seems to be
sneaked in accidentally (Oleg pointed me to this moment).

Reported-by: Alexey Dobriyan <adobriyan@...il.com>
CC: Oleg Nesterov <oleg@...hat.com>
CC: Andrey Vagin <avagin@...il.com>
CC: Dmitry Safonov <0x7f454c46@...il.com>
CC: Andrew Morton <akpm@...ux-foundation.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@...il.com>
---
Take a look please, once time permit. The issue on its own
should not be that critical but better to fix it. I tested
it manually via trivial test but I think it is not enough.
Need to implement some selftesting as well. Also obviously
I ran test on x86 only.

 kernel/sys.c |   73 +++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 32 deletions(-)

Index: linux-tip.git/kernel/sys.c
===================================================================
--- linux-tip.git.orig/kernel/sys.c
+++ linux-tip.git/kernel/sys.c
@@ -1961,6 +1961,33 @@ out:
 	return error;
 }
 
+static int copy_auxv_from_user(unsigned long *auxv, size_t auxv_size,
+			       const void __user *addr, size_t len)
+{
+	BUG_ON(auxv_size != sizeof(current->mm->saved_auxv));
+
+	if (!addr || len > auxv_size)
+		return -EINVAL;
+
+	memset(auxv, 0, auxv_size);
+	if (len && copy_from_user(auxv, addr, len))
+		return -EFAULT;
+
+	/*
+	 * Specification requires the vector to be
+	 * ended up with AT_NULL entry thus userspace
+	 * will notice where to stop enumerating. Thus
+	 * if someone is passing a screwed data make sure
+	 * at least it has the end of vector sign.
+	 */
+	if (len == auxv_size) {
+		if (__BITS_PER_LONG == 64)
+			auxv[AT_VECTOR_SIZE - 2] = AT_NULL;
+		auxv[AT_VECTOR_SIZE - 1] = AT_NULL;
+	}
+	return 0;
+}
+
 #ifdef CONFIG_CHECKPOINT_RESTORE
 static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data_size)
 {
@@ -1987,22 +2014,12 @@ static int prctl_set_mm_map(int opt, con
 		return error;
 
 	if (prctl_map.auxv_size) {
-		/*
-		 * Someone is trying to cheat the auxv vector.
-		 */
-		if (!prctl_map.auxv ||
-				prctl_map.auxv_size > sizeof(mm->saved_auxv))
-			return -EINVAL;
-
-		memset(user_auxv, 0, sizeof(user_auxv));
-		if (copy_from_user(user_auxv,
-				   (const void __user *)prctl_map.auxv,
-				   prctl_map.auxv_size))
-			return -EFAULT;
-
-		/* Last entry must be AT_NULL as specification requires */
-		user_auxv[AT_VECTOR_SIZE - 2] = AT_NULL;
-		user_auxv[AT_VECTOR_SIZE - 1] = AT_NULL;
+		int error = copy_auxv_from_user(user_auxv,
+						sizeof(user_auxv),
+						prctl_map.auxv,
+						prctl_map.auxv_size);
+		if (error)
+			return error;
 	}
 
 	if (prctl_map.exe_fd != (u32)-1) {
@@ -2079,25 +2096,17 @@ static int prctl_set_auxv(struct mm_stru
 	 * up to the caller to provide sane values here, otherwise userspace
 	 * tools which use this vector might be unhappy.
 	 */
-	unsigned long user_auxv[AT_VECTOR_SIZE] = {};
-
-	if (len > sizeof(user_auxv))
-		return -EINVAL;
-
-	if (copy_from_user(user_auxv, (const void __user *)addr, len))
-		return -EFAULT;
-
-	/* Make sure the last entry is always AT_NULL */
-	user_auxv[AT_VECTOR_SIZE - 2] = 0;
-	user_auxv[AT_VECTOR_SIZE - 1] = 0;
+	unsigned long user_auxv[AT_VECTOR_SIZE];
+	int error;
 
 	BUILD_BUG_ON(sizeof(user_auxv) != sizeof(mm->saved_auxv));
 
-	task_lock(current);
-	memcpy(mm->saved_auxv, user_auxv, len);
-	task_unlock(current);
-
-	return 0;
+	error = copy_auxv_from_user(user_auxv, sizeof(user_auxv),
+				    (const void __user *)addr,
+				    len);
+	if (!error)
+		memcpy(mm->saved_auxv, user_auxv, len);
+	return error;
 }
 
 static int prctl_set_mm(int opt, unsigned long addr,