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 stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html