Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 24 Mar 2021 01:06:44 +0300
From:   Cyrill Gorcunov <gorcunov@gmail.com>
To:     LKML <linux-kernel@vger.kernel.org>
Cc:     Alexey Dobriyan <adobriyan@gmail.com>,
        Oleg Nesterov <oleg@redhat.com>,
        Andrey Vagin <avagin@gmail.com>,
        Dmitry Safonov <0x7f454c46@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>
Subject: [PATCH v2] prctl: PR_SET_MM - unify copying of user's auvx
Message-ID: <YFpmdGXmGQ6IetoX@grain>
References: <YFeuWsnb6qKEU8+h@grain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YFeuWsnb6qKEU8+h@grain>
User-Agent: Mutt/2.0.5 (2021-01-21)
Precedence: bulk

prctl(PR_SET_MM, PR_SET_MM_AUXV | PR_SET_MM_MAP, ...) copies user
provided auxiliary vector to kernel and saves it to mm::saved_auxv,
this involves same code in to places. Lets move it into one helper
instead.

When we copy data from user space we make sure that the vector ends
up with AT_NULL key/value pair as specification requires. And here
is a bit vague moment if task is running in compat mode: instead of
one last value we zeroing two entries at the end. This is done for
code simplicity (if arch supports compat mode then the initial vector
size must be big enough to store values needed for the native mode
as well, that's why we define the vector as an array of longs. In
particular when Elf executable is loaded the vector is considered
as pairs of elf_addr_t elements, which is 4 byte per each on 32
bit environment and 8 byte per each in 64 bit kernel).

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 this moment).

Reported-by: Alexey Dobriyan <adobriyan@gmail.com>
Reported-by: Oleg Nesterov <oleg@redhat.com>
CC: Andrey Vagin <avagin@gmail.com>
CC: Dmitry Safonov <0x7f454c46@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 kernel/sys.c |   70 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 38 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,30 @@ 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 so user space
+	 * will notice where to stop enumerating.
+	 */
+	if (len == auxv_size) {
+		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 +2011,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 +2093,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,