From patchwork Sun Dec 27 02:28:31 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Al Viro X-Patchwork-Id: 7922891 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 802E3BEEE5 for ; Sun, 27 Dec 2015 02:29:00 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 4F260202DD for ; Sun, 27 Dec 2015 02:28:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 209F7202B8 for ; Sun, 27 Dec 2015 02:28:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754245AbbL0C2g (ORCPT ); Sat, 26 Dec 2015 21:28:36 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:54560 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753574AbbL0C2f (ORCPT ); Sat, 26 Dec 2015 21:28:35 -0500 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.76 #1 (Red Hat Linux)) id 1aD14R-0003Ky-Uz; Sun, 27 Dec 2015 02:28:31 +0000 Date: Sun, 27 Dec 2015 02:28:31 +0000 From: Al Viro To: Trond Myklebust Cc: Donald Buczek , Linux NFS Mailing List , Anna Schumaker Subject: Re: [PATCH] nfs: do not deny execute access based on outdated mode in inode Message-ID: <20151227022831.GF20997@ZenIV.linux.org.uk> References: <1451046656-26319-1-git-send-email-buczek@molgen.mpg.de> <567F29A7.2020906@molgen.mpg.de> <20151227003837.GE20997@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Sat, Dec 26, 2015 at 08:26:13PM -0500, Trond Myklebust wrote: > The may_open() is now happening before NFS gets a chance to issue the > OPEN rpc call, which is a change w.r.t. the lookup intent code. The > ordering is significant because it means the OPEN can no longer prime > the access cache. Always had... Consider e.g. device nodes; there ->open() might very well have side effects, and ones you do not want to allow for any random caller. Permissions checks always had been done prior to ->open(), it's not something new. > >> > Merry Christmas > >> > >> Suggestions Al? > > > > Make nfs_permission() relax the checks when it sees MAY_OPEN, if you know > > that things will be caught by server anyway? > > That can work as long as we're guaranteed that everything that calls > inode_permission() with MAY_OPEN on a regular file will also follow up > with a vfs_open() or dentry_open() on success. Is this always the > case? 1) in do_tmpfile(), followed by do_dentry_open() (not reachable by NFS since it doesn't have ->tmpfile() instance anyway) 2) in atomic_open(), after the call of ->atomic_open() has succeeded. 3) in do_last(), followed on success by vfs_open() That's all. All calls of inode_permission() that get MAY_OPEN come from may_open(), and there's no other callers of that puppy. PS: I'm not sure we want to carry that MAY_OPEN in op->acc_mode, actually. may_open() gets acc_mode without MAY_OPEN only when called from do_last() in case of O_PATH open. The check in the very beginning of may_open() triggers only in that case and might as well be replaced with if (likely(!(open_flag & O_PATH))) { error = may_open(&nd->path, acc_mode, open_flag); if (error) goto out; } in that call site (one right after finish_open_created:). Then we could bloody well have may_open() do error = inode_permission(inode, acc_mode | MAY_OPEN); and forget about MAY_OPEN in op->acc_mode. Something like the patch below should be an equivalent transformation and with that it's really clear what's going on with MAY_OPEN. Warning: completely untested. --- To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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/exec.c b/fs/exec.c index b06623a..828ec5f 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -119,7 +119,7 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) int error = PTR_ERR(tmp); static const struct open_flags uselib_flags = { .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC, - .acc_mode = MAY_READ | MAY_EXEC | MAY_OPEN, + .acc_mode = MAY_READ | MAY_EXEC, .intent = LOOKUP_OPEN, .lookup_flags = LOOKUP_FOLLOW, }; @@ -763,7 +763,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) int err; struct open_flags open_exec_flags = { .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC, - .acc_mode = MAY_EXEC | MAY_OPEN, + .acc_mode = MAY_EXEC, .intent = LOOKUP_OPEN, .lookup_flags = LOOKUP_FOLLOW, }; diff --git a/fs/namei.c b/fs/namei.c index 9e102ac..45c702e 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2663,10 +2663,6 @@ static int may_open(struct path *path, int acc_mode, int flag) struct inode *inode = dentry->d_inode; int error; - /* O_PATH? */ - if (!acc_mode) - return 0; - if (!inode) return -ENOENT; @@ -2688,7 +2684,7 @@ static int may_open(struct path *path, int acc_mode, int flag) break; } - error = inode_permission(inode, acc_mode); + error = inode_permission(inode, MAY_OPEN | acc_mode); if (error) return error; @@ -2880,7 +2876,7 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry, if (*opened & FILE_CREATED) { WARN_ON(!(open_flag & O_CREAT)); fsnotify_create(dir, dentry); - acc_mode = MAY_OPEN; + acc_mode = 0; } error = may_open(&file->f_path, acc_mode, open_flag); if (error) @@ -3093,7 +3089,7 @@ retry_lookup: /* Don't check for write permission, don't truncate */ open_flag &= ~O_TRUNC; will_truncate = false; - acc_mode = MAY_OPEN; + acc_mode = 0; path_to_nameidata(&path, nd); goto finish_open_created; } @@ -3177,10 +3173,11 @@ finish_open: got_write = true; } finish_open_created: - error = may_open(&nd->path, acc_mode, open_flag); - if (error) - goto out; - + if (likely(!(open_flag & O_PATH))) { + error = may_open(&nd->path, acc_mode, open_flag); + if (error) + goto out; + } BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */ error = vfs_open(&nd->path, file, current_cred()); if (!error) { @@ -3267,7 +3264,7 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags, goto out2; audit_inode(nd->name, child, 0); /* Don't check for other permissions, the inode was just created */ - error = may_open(&path, MAY_OPEN, op->open_flag); + error = may_open(&path, 0, op->open_flag); if (error) goto out2; file->f_path.mnt = path.mnt; diff --git a/fs/open.c b/fs/open.c index b6f1e96..b25b154 100644 --- a/fs/open.c +++ b/fs/open.c @@ -887,7 +887,7 @@ EXPORT_SYMBOL(dentry_open); static inline int build_open_flags(int flags, umode_t mode, struct open_flags *op) { int lookup_flags = 0; - int acc_mode; + int acc_mode = ACC_MODE(flags); if (flags & (O_CREAT | __O_TMPFILE)) op->mode = (mode & S_IALLUGO) | S_IFREG; @@ -909,7 +909,6 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o if (flags & __O_TMPFILE) { if ((flags & O_TMPFILE_MASK) != O_TMPFILE) return -EINVAL; - acc_mode = MAY_OPEN | ACC_MODE(flags); if (!(acc_mode & MAY_WRITE)) return -EINVAL; } else if (flags & O_PATH) { @@ -919,8 +918,6 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o */ flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH; acc_mode = 0; - } else { - acc_mode = MAY_OPEN | ACC_MODE(flags); } op->open_flag = flags;