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@xxxxxxxxxxxxxxx
Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
