* [PATCH 0/2] fs: copy_mount_options improvements
@ 2016-06-10 20:11 Andy Lutomirski
2016-06-10 20:11 ` [PATCH 1/2] fs: Improve and simplify copy_mount_options Andy Lutomirski
2016-06-10 20:11 ` [PATCH 2/2] fs: Disallow mount options strings longer than PAGE_SIZE - 1 Andy Lutomirski
0 siblings, 2 replies; 3+ messages in thread
From: Andy Lutomirski @ 2016-06-10 20:11 UTC (permalink / raw)
To: Linux FS Devel, Al Viro; +Cc: Stephen Rothwell, Andrew Morton, Andy Lutomirski
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(-)
--
2.5.5
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1/2] fs: Improve and simplify copy_mount_options
2016-06-10 20:11 [PATCH 0/2] fs: copy_mount_options improvements Andy Lutomirski
@ 2016-06-10 20:11 ` Andy Lutomirski
2016-06-10 20:11 ` [PATCH 2/2] fs: Disallow mount options strings longer than PAGE_SIZE - 1 Andy Lutomirski
1 sibling, 0 replies; 3+ messages in thread
From: Andy Lutomirski @ 2016-06-10 20:11 UTC (permalink / raw)
To: Linux FS Devel, Al Viro
Cc: Stephen Rothwell, Andrew Morton, Andy Lutomirski, stable
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 | 58 ++++++++++++----------------------------------------------
1 file changed, 12 insertions(+), 46 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691b4355..dfb5f370f2fa 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)
-{
- 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;
}
--
2.5.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] fs: Disallow mount options strings longer than PAGE_SIZE - 1
2016-06-10 20:11 [PATCH 0/2] fs: copy_mount_options improvements Andy Lutomirski
2016-06-10 20:11 ` [PATCH 1/2] fs: Improve and simplify copy_mount_options Andy Lutomirski
@ 2016-06-10 20:11 ` Andy Lutomirski
1 sibling, 0 replies; 3+ messages in thread
From: Andy Lutomirski @ 2016-06-10 20:11 UTC (permalink / raw)
To: Linux FS Devel, Al Viro; +Cc: Stephen Rothwell, Andrew Morton, 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 <luto@kernel.org>
---
fs/namespace.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index dfb5f370f2fa..0467f461dbd8 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -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)
--
2.5.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-06-10 20:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-10 20:11 [PATCH 0/2] fs: copy_mount_options improvements Andy Lutomirski
2016-06-10 20:11 ` [PATCH 1/2] fs: Improve and simplify copy_mount_options Andy Lutomirski
2016-06-10 20:11 ` [PATCH 2/2] fs: Disallow mount options strings longer than PAGE_SIZE - 1 Andy Lutomirski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).