From patchwork Thu Jun 16 06:57:38 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Willy Tarreau X-Patchwork-Id: 9179909 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 35B0360776 for ; Thu, 16 Jun 2016 06:57:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2405127BF7 for ; Thu, 16 Jun 2016 06:57:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1704A2804C; Thu, 16 Jun 2016 06:57:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 626FA27BF7 for ; Thu, 16 Jun 2016 06:57:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752158AbcFPG5w (ORCPT ); Thu, 16 Jun 2016 02:57:52 -0400 Received: from wtarreau.pck.nerim.net ([62.212.114.60]:50702 "EHLO 1wt.eu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751275AbcFPG5v (ORCPT ); Thu, 16 Jun 2016 02:57:51 -0400 Received: (from willy@localhost) by pcw.home.local (8.15.2/8.15.2/Submit) id u5G6vcaY007298; Thu, 16 Jun 2016 08:57:38 +0200 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-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- 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 --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;