fs: copy_mount_options improvements

PATCH Superseded Andy Lutomirski 1 participant 3 messages Jun 10, 2016

Subsystems (2)

Stable branch VFS

lore

Share & tools

lore

Andy Lutomirski

author

2016-06-10 20:11 UTC #0 lore raw

cover fs: copy_mount_options improvements

Patch 1 fixes what is arguably a bug. I suspect it could cause crashes or very large latency under unusual circumstanes. Since it also deletes a bunch of code, I think it's a nice fix.

Patch 2 is an ABI change/fix. Any user code that would be affected is currently buggy (i.e. results in incorrect option parsing), so I suspect it can be applied safely.

Andy Lutomirski (2):
fs: Improve and simplify copy_mount_options
fs: Disallow mount options strings longer than PAGE_SIZE - 1

 fs/namespace.c | 60 +++++++++++++---------------------------------------------
 1 file changed, 13 insertions(+), 47 deletions(-)

Signature · 1 line

2.5.5

Andy Lutomirski

author

2016-06-10 20:11 UTC #1 lore raw

PATCH 1/2 fs: Improve and simplify copy_mount_options

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 [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/namespace.c | 58 ++++++++++++----------------------------------------------
 1 file changed, 12 insertions(+), 46 deletions(-)

fs/namespace.c +12 ~~-46~~

index 4fb1691b4355..dfb5f370f2fa 100644
@@ -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)
-{
-	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)
+void *copy_mount_options(const void __user *data)
 {
-	int i;
-	unsigned long size;
+	long size;
 	char *copy;
 
 	if (!data)

@@ -2622,22 +2597,13 @@ 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) {
-		kfree(copy);
-		return ERR_PTR(-EFAULT);
-	}
-	if (i != PAGE_SIZE)
-		memset(copy + i, 0, PAGE_SIZE - i);
+	size = strncpy_from_user(copy, data, PAGE_SIZE);
+	if (size < 0)
+		return ERR_PTR(size);
+
+	/* If we got less than PAGE_SIZE bytes, zero out the remainder. */
+	memset(copy + size, 0, PAGE_SIZE);
+
 	return copy;
 }

Signature · 1 line

2.5.5

Andy Lutomirski

author

2016-06-10 20:11 UTC #2 lore raw

PATCH 2/2 fs: Disallow mount options strings longer than PAGE_SIZE - 1

In reply to Andy Lutomirski

We used to truncate the string. Make the behaviour of mount() more
predictable: return -EINVAL if the string is too long.

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/namespace.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

fs/namespace.c +7 -7

index dfb5f370f2fa..0467f461dbd8 100644
@@ -2582,8 +2582,7 @@ static void shrink_submounts(struct mount *mnt)
 }
 
 /* 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.
+ * with nulls and guarantees that the result is null-terminated.
  */
 void *copy_mount_options(const void __user *data)
 {

@@ -2601,7 +2600,12 @@ void *copy_mount_options(const void __user *data)
 	if (size < 0)
 		return ERR_PTR(size);
 
-	/* If we got less than PAGE_SIZE bytes, zero out the remainder. */
+	if (size >= PAGE_SIZE) {
+		kfree(copy);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* Pad with zeros. */
 	memset(copy + size, 0, PAGE_SIZE);
 
 	return copy;

@@ -2637,10 +2641,6 @@ long do_mount(const char *dev_name, const char __user *dir_name,
 	if ((flags & MS_MGC_MSK) == MS_MGC_VAL)
 		flags &= ~MS_MGC_MSK;
 
-	/* Basic sanity checks */
-	if (data_page)
-		((char *)data_page)[PAGE_SIZE - 1] = 0;
-
 	/* ... and get the mountpoint */
 	retval = user_path(dir_name, &path);
 	if (retval)

Signature · 1 line

2.5.5