[PATCH v2 1/2] fs: Improve and simplify copy_mount_options

[Andy Lutomirski <luto@kernel.org>]

copy_mount_options always tries to copy a full page even if the
string is shorter than a page.  If the string starts part-way into a
page and ends on the same page it started on, this means that
copy_mount_options can overrun the supplied buffer and read into the
next page.

If the buffer came from userspace (USER_DS), then this could be a
performance issue (reading across the page boundary could block).
If the buffer came from the kernel (KERNEL_DS), then this could read
an unrelated page, and the kernel can have pages mapped in that have
side-effects.

I noticed this due to a new sanity-check I'm working on that tries
to make sure that we don't try to access nonexistent pages under
KERNEL_DS.

This is the same issue that was fixed by commit eca6f534e619 ("fs:
fix overflow in sys_mount() for in-kernel calls"), but for
copy_mount_options instead of copy_mount_string.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 fs/namespace.c | 56 ++++++++++++--------------------------------------------
 1 file changed, 12 insertions(+), 44 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691b4355..8644f1961ca6 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2581,38 +2581,13 @@ static void shrink_submounts(struct mount *mnt)
 	}
 }
 
-/*
- * Some copy_from_user() implementations do not return the exact number of
- * bytes remaining to copy on a fault.  But copy_mount_options() requires that.
- * Note that this function differs from copy_from_user() in that it will oops
- * on bad values of `to', rather than returning a short copy.
+/* Copy the mount options string.  Always returns a full page padded
+ * with nulls.  If the input string is a full page or more, it may be
+ * truncated and the result will not be null-terminated.
  */
-static long exact_copy_from_user(void *to, const void __user * from,
-				 unsigned long n)
+void *copy_mount_options(const void __user *data)
 {
-	char *t = to;
-	const char __user *f = from;
-	char c;
-
-	if (!access_ok(VERIFY_READ, from, n))
-		return n;
-
-	while (n) {
-		if (__get_user(c, f)) {
-			memset(t, 0, n);
-			break;
-		}
-		*t++ = c;
-		f++;
-		n--;
-	}
-	return n;
-}
-
-void *copy_mount_options(const void __user * data)
-{
-	int i;
-	unsigned long size;
+	long size;
 	char *copy;
 
 	if (!data)
@@ -2622,22 +2597,15 @@ void *copy_mount_options(const void __user * data)
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
 
-	/* We only care that *some* data at the address the user
-	 * gave us is valid.  Just in case, we'll zero
-	 * the remainder of the page.
-	 */
-	/* copy_from_user cannot cross TASK_SIZE ! */
-	size = TASK_SIZE - (unsigned long)data;
-	if (size > PAGE_SIZE)
-		size = PAGE_SIZE;
-
-	i = size - exact_copy_from_user(copy, data, size);
-	if (!i) {
+	size = strncpy_from_user(copy, data, PAGE_SIZE);
+	if (size < 0) {
 		kfree(copy);
-		return ERR_PTR(-EFAULT);
+		return ERR_PTR(size);
 	}
-	if (i != PAGE_SIZE)
-		memset(copy + i, 0, PAGE_SIZE - i);
+
+	/* If we got less than PAGE_SIZE bytes, zero out the remainder. */
+	memset(copy + size, 0, PAGE_SIZE - size);
+
 	return copy;
 }
 
-- 
2.7.4