Skip to contentSkip to conversation

copy_mount_options improvements

PATCH In review Andy Lutomirski 5 participants 16 messages Jun 14, 2016 – Jun 17, 2016

Subsystems (2)
Stable branch VFS
lore
Share & tools
lore

Andy Lutomirski

author
#0 lore raw

v2 cover 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.

Changes from v1:
- Fix some blatant memory corruption :(

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

 fs/namespace.c | 62 ++++++++++++++--------------------------------------------
 1 file changed, 15 insertions(+), 47 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH v2 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 | 56 ++++++++++++--------------------------------------------
 1 file changed, 12 insertions(+), 44 deletions(-)
fs/namespace.c +12 -44
index 4fb1691b4355..8644f1961ca6 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)
+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;
 }
Signature · 1 line
2.7.4
maintainer
#2 lore raw

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

Quoted 5 lines · Andy Lutomirski

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.

Have you considered the possibility that there might be a reason for having separate copy_mount_option() and copy_mount_string()? Such as options not being a string, perhaps?

In some filesystems (including older NFS variants) it is not a string at all - a binary data structure, with quite a few zero bytes in it. And no, we fucking *can't* break mount.nfs(8), no matter how we'd like to get rid of that wart of an ABI.

IOW, NAK with prejudice - don't bring that thing back, it's hard no-go.

#3 lore raw

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

Quoted 11 lines · Al Viro
Quoted 5 lines · Andy Lutomirski

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.

Have you considered the possibility that there might be a reason for having separate copy_mount_option() and copy_mount_string()? Such as options not being a string, perhaps?

Quoted 6 lines

In some filesystems (including older NFS variants) it is not a string at all - a binary data structure, with quite a few zero bytes in it. And no, we fucking *can't* break mount.nfs(8), no matter how we'd like to get rid of that wart of an ABI.

IOW, NAK with prejudice - don't bring that thing back, it's hard no-go.

Well, that sucks. I suppose we could make it conditional on the fs type being "nfs", but yuck.

If we don't fix this, though, then we have other problems:

devtmpfsd does:

        *err = sys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, options);

where options points to the kernel stack. This is bad. do_mount_root is similarly broken.

Is there any reason that these things use sys_mount instead of do_mount?

--Andy

#4 lore raw

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

Quoted 9 lines · Andy Lutomirski

devtmpfsd does:

        *err = sys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, options);

where options points to the kernel stack. This is bad. do_mount_root is similarly broken.

Is there any reason that these things use sys_mount instead of do_mount?

Not that I can see. But maybe copy_mount_options could also check for KERNEL_DS, and use a strncpy instead of a copy_from_user() for that case?

Linus

#5 lore raw

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

Quoted 6 lines · Linus Torvalds
Quoted 5 lines · Andy Lutomirski

devtmpfsd does:

        *err = sys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, options);
Quoted 8 lines
Quoted 4 lines

where options points to the kernel stack. This is bad. do_mount_root is similarly broken.

Is there any reason that these things use sys_mount instead of do_mount?

Not that I can see. But maybe copy_mount_options could also check for KERNEL_DS, and use a strncpy instead of a copy_from_user() for that case?

Well, strncpy() would make the function behave differently depending on the FS being used if called from the kernel for the reason Al mentionned. OK devtmpfsd() passes a string, but if it's the FS itself which decides to stop on a zero when parsing mount options, we'd probably rather use memcpy() instead to ensure a consistent behaviour, like this maybe ?

Willy

fs/namespace.c +6 -0
index 4fb1691..058b856 100644
@@ -2622,6 +2622,12 @@ void *copy_mount_options(const void __user * data)
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
 
+	/* do_mount() may be called from the kernel */
+	if (segment_eq(get_fs(), KERNEL_DS)) {
+		memcpy(copy, data, PAGE_SIZE);
+		return copy;
+	}
+
 	/* 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.
#6 lore raw

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

Quoted 6 lines · Willy Tarreau

Well, strncpy() would make the function behave differently depending on the FS being used if called from the kernel for the reason Al mentionned. OK devtmpfsd() passes a string, but if it's the FS itself which decides to stop on a zero when parsing mount options, we'd probably rather use memcpy() instead to ensure a consistent behaviour, like this maybe ?

.. but that is exactly what Andy considers to be a problem: now it copies random kernel memory that is possibly security-critical.

The kernel users that use this just pass in a string - it doesn't matter what the filesystem thinks it is getting, the uses were all kernel strings,, so the "copy_mount_options": should copy that string (and zero-fill the page that the filesystem may think it is getting).

Linus

#7 lore raw

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

Quoted 8 lines · Linus Torvalds
Quoted 6 lines · Willy Tarreau

Well, strncpy() would make the function behave differently depending on the FS being used if called from the kernel for the reason Al mentionned. OK devtmpfsd() passes a string, but if it's the FS itself which decides to stop on a zero when parsing mount options, we'd probably rather use memcpy() instead to ensure a consistent behaviour, like this maybe ?

Quoted 7 lines

.. but that is exactly what Andy considers to be a problem: now it copies random kernel memory that is possibly security-critical.

The kernel users that use this just pass in a string - it doesn't matter what the filesystem thinks it is getting, the uses were all kernel strings,, so the "copy_mount_options": should copy that string (and zero-fill the page that the filesystem may think it is getting).

But I still find it ugly to consider that if the options come from the kernel they're a zero-terminated string otherwise they're a page :-/ Couldn't we instead look up the fstype before calling copy_mount_options() ?

Quoted 1 line

From what I'm seeing, we already have FS_BINARY_MOUNTDATA in the FS type

to indicate that it expects binary mount options, so probably we could check it in copy_mount_options() if we pass it the fstype. Something approximately like this (not even build tested).

Willy

fs/compat.c +1 -1
index be6e48b..8494766 100644
@@ -806,7 +806,7 @@ COMPAT_SYSCALL_DEFINE5(mount, const char __user *, dev_name,
 	if (IS_ERR(kernel_dev))
 		goto out1;
 
-	options = copy_mount_options(data);
+	options = copy_mount_options(type, data);
 	retval = PTR_ERR(options);
 	if (IS_ERR(options))
 		goto out2;
fs/internal.h +1 -1
index b71deee..3c9bc7b 100644
@@ -55,7 +55,7 @@ extern int vfs_path_lookup(struct dentry *, struct vfsmount *,
 /*
  * namespace.c
  */
-extern void *copy_mount_options(const void __user *);
+extern void *copy_mount_options(const char __user *, const void __user *);
 extern char *copy_mount_string(const void __user *);
 
 extern struct vfsmount *lookup_mnt(struct path *);
fs/namespace.c +13 -2
index 4fb1691..cf28d08 100644
@@ -2609,19 +2609,30 @@ static long exact_copy_from_user(void *to, const void __user * from,
 	return n;
 }
 
-void *copy_mount_options(const void __user * data)
+void *copy_mount_options(const void __user *fstype, const void __user * data)
 {
 	int i;
 	unsigned long size;
 	char *copy;
+	struct file_system_type *type;
 
 	if (!data)
 		return NULL;
 
+	type = get_fs_type(fstype);
+	if (!type)
+		return NULL;
+
 	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
 
+	/* avoid reading a whole page if the FS only needs a string. */
+	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
+		strlcpy(copy, data, PAGE_SIZE);
+		return copy;
+	}
+
 	/* 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.

@@ -2917,7 +2928,7 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
 	if (IS_ERR(kernel_dev))
 		goto out_dev;
 
-	options = copy_mount_options(data);
+	options = copy_mount_options(type, data);
 	ret = PTR_ERR(options);
 	if (IS_ERR(options))
 		goto out_data;
#8 lore raw

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

On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
(...)

fs/namespace.c static long exact_copy_from_user(void *to, const void __user * from, · line 2632

Quoted 3 lines
fs/namespace.c +3 -0
+	/* avoid reading a whole page if the FS only needs a string. */
+	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
+		strlcpy(copy, data, PAGE_SIZE);

BTW, I forgot that we're first supposed to come from user, make that strndup_user() instead.

Willy

maintainer
#9 lore raw

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

In reply to Willy Tarreau

fs/namespace.c static long exact_copy_from_user(void *to, const void __user * from, · line 2633

Quoted 12 lines · Willy Tarreau
fs/namespace.c +8 -0
+	type = get_fs_type(fstype);
+	if (!type)
+		return NULL;
+
 	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
 
+	/* avoid reading a whole page if the FS only needs a string. */
+	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
+		strlcpy(copy, data, PAGE_SIZE);
+		return copy;

a) it leaks a file_system_type reference
b) data is a userland pointer, for crying out loud!

#10 lore raw

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

Quoted 9 lines · Al Viro
Quoted 8 lines · Willy Tarreau
fs/namespace.c +4 -0
+	type = get_fs_type(fstype);
+	if (!type)
+		return NULL;
+
 	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
Quoted 6 lines

fs/namespace.c static long exact_copy_from_user(void *to, const void __user * from, · line 2633

Quoted 4 lines
fs/namespace.c +4 -0
+	/* avoid reading a whole page if the FS only needs a string. */
+	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
+		strlcpy(copy, data, PAGE_SIZE);
+		return copy;

a) it leaks a file_system_type reference

I was not sure about this one, thanks for confirming.

Quoted 1 line

b) data is a userland pointer, for crying out loud!

Yep I noticed it and fixed it after sending. I was focused on the data coming from kernel due to the discussion.

I also think that since there are only two call places for copy_mount_options(), we may move the test there and switch to copy_mount_string() instead depending on the fs type.

Thanks,
Willy

maintainer
#11 lore raw

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

Quoted 19 lines · Willy Tarreau
Quoted 9 lines · Al Viro
Quoted 8 lines · Willy Tarreau
fs/namespace.c +4 -0
+	type = get_fs_type(fstype);
+	if (!type)
+		return NULL;
+
 	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
Quoted 6 lines

fs/namespace.c static long exact_copy_from_user(void *to, const void __user * from, · line 2633

Quoted 4 lines
fs/namespace.c +4 -0
+	/* avoid reading a whole page if the FS only needs a string. */
+	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
+		strlcpy(copy, data, PAGE_SIZE);
+		return copy;

a) it leaks a file_system_type reference

I was not sure about this one, thanks for confirming.

Quoted 8 lines
Quoted 1 line

b) data is a userland pointer, for crying out loud!

Yep I noticed it and fixed it after sending. I was focused on the data coming from kernel due to the discussion.

I also think that since there are only two call places for copy_mount_options(), we may move the test there and switch to copy_mount_string() instead depending on the fs type.

Another problem is that it will oops with NULL fstype, which is absolutely normal both for mount --bind *and* mount -o remount. And while mount --bind doesn't care about string options, mount -o remount certainly does. IMO the latter makes that approach hopeless - with remount you don't know the type until well into do_mount() guts and I'd really hate to carry the userland pointer all the way into it.

#12 lore raw

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

Quoted 25 lines · Al Viro
Quoted 24 lines · Willy Tarreau
Quoted 9 lines · Al Viro
Quoted 8 lines · Willy Tarreau
fs/namespace.c +4 -0
+	type = get_fs_type(fstype);
+	if (!type)
+		return NULL;
+
 	copy = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
Quoted 6 lines

fs/namespace.c static long exact_copy_from_user(void *to, const void __user * from, · line 2633

Quoted 4 lines
fs/namespace.c +4 -0
+	/* avoid reading a whole page if the FS only needs a string. */
+	if (!(type->fs_flags & FS_BINARY_MOUNTDATA)) {
+		strlcpy(copy, data, PAGE_SIZE);
+		return copy;

a) it leaks a file_system_type reference

I was not sure about this one, thanks for confirming.

Quoted 1 line

b) data is a userland pointer, for crying out loud!

Yep I noticed it and fixed it after sending. I was focused on the data coming from kernel due to the discussion.

Quoted 6 lines
Quoted 3 lines

I also think that since there are only two call places for copy_mount_options(), we may move the test there and switch to copy_mount_string() instead depending on the fs type.

Another problem is that it will oops with NULL fstype, which is absolutely normal both for mount --bind *and* mount -o remount.

OK thanks for explaining, I didn't know.

Quoted 5 lines

And while mount --bind doesn't care about string options, mount -o remount certainly does. IMO the latter makes that approach hopeless - with remount you don't know the type until well into do_mount() guts and I'd really hate to carry the userland pointer all the way into it.

Agreed. However if the initial point was to avoid reading extra pages most of the time, we could possibly consider that the string copy is an optimization for the case where we have the information available. Thus if !fstype || !(type->fs_flags & FS_BINARY_MOUNTDATA) then we use copy_mount_options() otherwise we use copy_mount_string(). It will only leave the full page copy for mount --bind or -o remount then.

Willy

#13 lore raw

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

Quoted 7 lines · Willy Tarreau

However if the initial point was to avoid reading extra pages most of the time, we could possibly consider that the string copy is an optimization for the case where we have the information available. Thus if !fstype || !(type->fs_flags & FS_BINARY_MOUNTDATA) then we use copy_mount_options() otherwise we use copy_mount_string(). It will only leave the full page copy for mount --bind or -o remount then.

For example like this (not compile-tested either) :

fs/compat.c +9 -1
index be6e48b..6407d40 100644
@@ -795,18 +795,26 @@ COMPAT_SYSCALL_DEFINE5(mount, const char __user *, dev_name,
 	void *options;
 	char *kernel_dev;
 	int retval;
+	bool use_str = false;
+	struct file_system_type *fstype;
 
 	kernel_type = copy_mount_string(type);
 	retval = PTR_ERR(kernel_type);
 	if (IS_ERR(kernel_type))
 		goto out;
 
+	if (kernel_type && (fstype = get_fs_type(kernel_type)) {
+		if (!(fstype->fs_flags & FS_BINARY_MOUNTDATA))
+			use_str = true;
+		put_filesystem(fstype);
+	}
+
 	kernel_dev = copy_mount_string(dev_name);
 	retval = PTR_ERR(kernel_dev);
 	if (IS_ERR(kernel_dev))
 		goto out1;
 
-	options = copy_mount_options(data);
+	options = use_str ? copy_mount_string(data) : copy_mount_options(data);
 	retval = PTR_ERR(options);
 	if (IS_ERR(options))
 		goto out2;
fs/namespace.c +9 -1
index 4fb1691..f14089d 100644
@@ -2906,18 +2906,26 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
 	char *kernel_type;
 	char *kernel_dev;
 	void *options;
+	bool use_str = false;
+	struct file_system_type *fstype;
 
 	kernel_type = copy_mount_string(type);
 	ret = PTR_ERR(kernel_type);
 	if (IS_ERR(kernel_type))
 		goto out_type;
 
+	if (kernel_type && (fstype = get_fs_type(kernel_type)) {
+		if (!(fstype->fs_flags & FS_BINARY_MOUNTDATA))
+			use_str = true;
+		put_filesystem(fstype);
+	}
+
 	kernel_dev = copy_mount_string(dev_name);
 	ret = PTR_ERR(kernel_dev);
 	if (IS_ERR(kernel_dev))
 		goto out_dev;
 
-	options = copy_mount_options(data);
+	options = use_str ? copy_mount_string(data) : copy_mount_options(data);
 	ret = PTR_ERR(options);
 	if (IS_ERR(options))
 		goto out_data;
#14 lore raw

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

Quoted 10 lines · Willy Tarreau
Quoted 7 lines · Willy Tarreau

However if the initial point was to avoid reading extra pages most of the time, we could possibly consider that the string copy is an optimization for the case where we have the information available. Thus if !fstype || !(type->fs_flags & FS_BINARY_MOUNTDATA) then we use copy_mount_options() otherwise we use copy_mount_string(). It will only leave the full page copy for mount --bind or -o remount then.

For example like this (not compile-tested either) :

I like this approach. Al, is this okay with you?

--Andy

Quoted 65 lines
fs/compat.c +9 -1
index be6e48b..6407d40 100644
@@ -795,18 +795,26 @@ COMPAT_SYSCALL_DEFINE5(mount, const char __user *, dev_name,
        void *options;
        char *kernel_dev;
        int retval;
+       bool use_str = false;
+       struct file_system_type *fstype;
 
        kernel_type = copy_mount_string(type);
        retval = PTR_ERR(kernel_type);
        if (IS_ERR(kernel_type))
                goto out;
 
+       if (kernel_type && (fstype = get_fs_type(kernel_type)) {
+               if (!(fstype->fs_flags & FS_BINARY_MOUNTDATA))
+                       use_str = true;
+               put_filesystem(fstype);
+       }
+
        kernel_dev = copy_mount_string(dev_name);
        retval = PTR_ERR(kernel_dev);
        if (IS_ERR(kernel_dev))
                goto out1;
 
-       options = copy_mount_options(data);
+       options = use_str ? copy_mount_string(data) : copy_mount_options(data);
        retval = PTR_ERR(options);
        if (IS_ERR(options))
                goto out2;
fs/namespace.c +9 -1
index 4fb1691..f14089d 100644
@@ -2906,18 +2906,26 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
        char *kernel_type;
        char *kernel_dev;
        void *options;
+       bool use_str = false;
+       struct file_system_type *fstype;
 
        kernel_type = copy_mount_string(type);
        ret = PTR_ERR(kernel_type);
        if (IS_ERR(kernel_type))
                goto out_type;
 
+       if (kernel_type && (fstype = get_fs_type(kernel_type)) {
+               if (!(fstype->fs_flags & FS_BINARY_MOUNTDATA))
+                       use_str = true;
+               put_filesystem(fstype);
+       }
+
        kernel_dev = copy_mount_string(dev_name);
        ret = PTR_ERR(kernel_dev);
        if (IS_ERR(kernel_dev))
                goto out_dev;
 
-       options = copy_mount_options(data);
+       options = use_str ? copy_mount_string(data) : copy_mount_options(data);
        ret = PTR_ERR(options);
        if (IS_ERR(options))
                goto out_data;
Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
author
#15 lore raw

PATCH v2 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 8644f1961ca6..96e6b09df7d1 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)
 {

@@ -2603,7 +2602,12 @@ void *copy_mount_options(const void __user *data)
 		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 - size);
 
 	return copy;

@@ -2639,10 +2643,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.7.4