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

[Willy Tarreau <w@1wt.eu>]

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

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;