diff mbox

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

Message ID 20160616065738.GA7154@1wt.eu (mailing list archive)
State New, archived
Headers show

Commit Message

Willy Tarreau June 16, 2016, 6:57 a.m. UTC 
On Wed, Jun 15, 2016 at 07:59:00PM -1000, Linus Torvalds wrote:
> On Wed, Jun 15, 2016 at 7:45 PM, Willy Tarreau <w@1wt.eu> wrote:
> >
> > 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).

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() ?
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


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Willy Tarreau June 16, 2016, 7:02 a.m. UTC | #1 
On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
(...)
> +	/* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro June 16, 2016, 7:08 a.m. UTC | #2 
On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
> +	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!
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Willy Tarreau June 16, 2016, 7:25 a.m. UTC | #3 
On Thu, Jun 16, 2016 at 08:08:22AM +0100, Al Viro wrote:
> On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
> > +	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

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

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro June 16, 2016, 8:02 a.m. UTC | #4 
On Thu, Jun 16, 2016 at 09:25:57AM +0200, Willy Tarreau wrote:
> On Thu, Jun 16, 2016 at 08:08:22AM +0100, Al Viro wrote:
> > On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
> > > +	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
> 
> I was not sure about this one, thanks for confirming.
> 
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Willy Tarreau June 16, 2016, 8:20 a.m. UTC | #5 
On Thu, Jun 16, 2016 at 09:02:29AM +0100, Al Viro wrote:
> On Thu, Jun 16, 2016 at 09:25:57AM +0200, Willy Tarreau wrote:
> > On Thu, Jun 16, 2016 at 08:08:22AM +0100, Al Viro wrote:
> > > On Thu, Jun 16, 2016 at 08:57:38AM +0200, Willy Tarreau wrote:
> > > > +	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
> > 
> > I was not sure about this one, thanks for confirming.
> > 
> > > 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.

OK thanks for explaining, I didn't know.

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/compat.c b/fs/compat.c
index be6e48b..8494766 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -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;
diff --git a/fs/internal.h b/fs/internal.h
index b71deee..3c9bc7b 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -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 *);
diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691..cf28d08 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -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;