From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 16 Jun 2016 08:57:38 +0200 From: Willy Tarreau To: Linus Torvalds Cc: Andy Lutomirski , Al Viro , Andy Lutomirski , Linux FS Devel , Stephen Rothwell , Andrew Morton , stable Subject: Re: [PATCH v2 1/2] fs: Improve and simplify copy_mount_options Message-ID: <20160616065738.GA7154@1wt.eu> References: <20160615235047.GA14480@ZenIV.linux.org.uk> <20160616054525.GA6803@1wt.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Jun 15, 2016 at 07:59:00PM -1000, Linus Torvalds wrote: > On Wed, Jun 15, 2016 at 7:45 PM, Willy Tarreau 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;