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@xxxxxx> 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;
--
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
