* [PATCH 3/3] NFSD - Use MAY_ACT_AS_OWNER
2018-10-04 1:02 [PATCH 0/3] Fix regression in NFSv3 ACL setting NeilBrown
@ 2018-10-04 1:02 ` NeilBrown
2018-10-04 1:02 ` [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER NeilBrown
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: NeilBrown @ 2018-10-04 1:02 UTC (permalink / raw)
To: J. Bruce Fields, Anna Schumaker, Alexander Viro, Trond Myklebust
Cc: Jan Harkes, linux-nfs, Miklos Szeredi, Jeff Layton, linux-kernel,
linux-afs, David Howells, coda, linux-fsdevel, Christoph Hellwig
The NFSD_MAY_OWNER_OVERRIDE has exactly the same meaning
as the new MAY_ACT_AS_OWNER flag, so simplify the
code by making use of the identity.
The move NFSD_MAY_OWNER_OVERRIDE into NFSD_MAY_MASK, but that
is not a problem is it is always set together with a flag
that is already in the MASK.
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/nfsd/vfs.c | 11 ++++++-----
fs/nfsd/vfs.h | 14 +++++++-------
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 55a099e47ba2..d89d23e6e2fe 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -2038,12 +2038,13 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
* We must trust the client to do permission checking - using "ACCESS"
* with NFSv3.
*/
- if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
- uid_eq(inode->i_uid, current_fsuid()))
- return 0;
- /* This assumes NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
- err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
+ /*
+ * This works as NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC}
+ * and NFSD_MAY_OWNER_OVERRIDE == MAY_ACT_AS_OWNER
+ */
+ err = inode_permission(inode, (acc & (MAY_READ|MAY_WRITE|
+ MAY_EXEC|MAY_ACT_AS_OWNER)));
/* Allow read access to binaries even when mode 111 */
if (err == -EACCES && S_ISREG(inode->i_mode) &&
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 2b1c70d3757a..f6e96dba76a5 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -16,6 +16,7 @@
#define NFSD_MAY_EXEC MAY_EXEC
#define NFSD_MAY_WRITE MAY_WRITE
#define NFSD_MAY_READ MAY_READ
+#define NFSD_MAY_OWNER_OVERRIDE MAY_ACT_AS_OWNER
#define NFSD_MAY_SATTR (__MAY_UNUSED << 0)
#define NFSD_MAY_TRUNC (__MAY_UNUSED << 1)
#define NFSD_MAY_LOCK (__MAY_UNUSED << 2)
@@ -23,16 +24,15 @@
#define NFSD_MAY_MASK (__NFSD_MAY_FIRST_HINT - 1)
/* extra hints to permission and open routines: */
-#define NFSD_MAY_OWNER_OVERRIDE (__NFSD_MAY_FIRST_HINT << 0)
/* for device special files */
-#define NFSD_MAY_LOCAL_ACCESS (__NFSD_MAY_FIRST_HINT << 1)
-#define NFSD_MAY_BYPASS_GSS_ON_ROOT (__NFSD_MAY_FIRST_HINT << 2)
-#define NFSD_MAY_NOT_BREAK_LEASE (__NFSD_MAY_FIRST_HINT << 3)
-#define NFSD_MAY_BYPASS_GSS (__NFSD_MAY_FIRST_HINT << 4)
-#define NFSD_MAY_READ_IF_EXEC (__NFSD_MAY_FIRST_HINT << 5)
+#define NFSD_MAY_LOCAL_ACCESS (__NFSD_MAY_FIRST_HINT << 0)
+#define NFSD_MAY_BYPASS_GSS_ON_ROOT (__NFSD_MAY_FIRST_HINT << 1)
+#define NFSD_MAY_NOT_BREAK_LEASE (__NFSD_MAY_FIRST_HINT << 2)
+#define NFSD_MAY_BYPASS_GSS (__NFSD_MAY_FIRST_HINT << 3)
+#define NFSD_MAY_READ_IF_EXEC (__NFSD_MAY_FIRST_HINT << 4)
/* 64 bit readdir cookies for >= NFSv3 */
-#define NFSD_MAY_64BIT_COOKIE (__NFSD_MAY_FIRST_HINT << 6)
+#define NFSD_MAY_64BIT_COOKIE (__NFSD_MAY_FIRST_HINT << 5)
#define NFSD_MAY_CREATE (NFSD_MAY_EXEC|NFSD_MAY_WRITE)
#define NFSD_MAY_REMOVE (NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER
2018-10-04 1:02 [PATCH 0/3] Fix regression in NFSv3 ACL setting NeilBrown
2018-10-04 1:02 ` [PATCH 3/3] NFSD - Use MAY_ACT_AS_OWNER NeilBrown
@ 2018-10-04 1:02 ` NeilBrown
2018-10-04 1:02 ` [PATCH 2/3] VFS: allow MAY_ flags to be easily extended NeilBrown
2018-10-04 14:10 ` [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER David Howells
3 siblings, 0 replies; 10+ messages in thread
From: NeilBrown @ 2018-10-04 1:02 UTC (permalink / raw)
To: J. Bruce Fields, Anna Schumaker, Alexander Viro, Trond Myklebust
Cc: Jan Harkes, linux-nfs, Miklos Szeredi, Jeff Layton, linux-kernel,
linux-afs, David Howells, coda, linux-fsdevel, Christoph Hellwig
A few places in VFS, particularly set_posix_acl(), use
inode_owner_or_capable() to check if the caller has "owner"
access to the inode.
This assumes that it is valid to test inode->i_uid, which is not
always the case. Particularly in the case of NFS it is not valid to
us i_uid (or i_mode) for permission tests - the server needs to make
the decision.
As a result if the server is remapping uids
(e.g. all-squash,anon_uid=1000),
then all users should have ownership access, but most users will not
be able to set acls.
This patch moves the ownership test to inode_permission and
i_op->permission.
A new flag for these functions, MAY_ACT_AS_OWNER is introduced.
generic_permission() now handles this correctly and many
i_op->permission functions call this function() and don't need any
changes. A few are changed to handle MAY_ACT_AS_OWNER exactly
as generic_permission() does, using inode_owner_or_capable().
For these filesystems, no behavioural change should be noticed.
For NFS, nfs_permission is changed to always return 0 (success) if
MAY_ACT_AS_OWNER. For NFS, any operations which use this flag should
be sent to the server, and the server will succeed or fail as
appropriate.
Fixes: 013cdf1088d7 ("nfs: use generic posix ACL infrastructure for v3 Posix ACLs")
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/afs/security.c | 10 ++++++++++
fs/attr.c | 12 +++++-------
fs/coda/dir.c | 10 ++++++++++
fs/fcntl.c | 2 +-
fs/fuse/dir.c | 10 ++++++++++
fs/namei.c | 9 +++++++++
fs/nfs/dir.c | 8 ++++++++
fs/posix_acl.c | 2 +-
fs/xattr.c | 2 +-
include/linux/fs.h | 8 ++++++++
10 files changed, 63 insertions(+), 10 deletions(-)
diff --git a/fs/afs/security.c b/fs/afs/security.c
index 81dfedb7879f..ac2e39de8bff 100644
--- a/fs/afs/security.c
+++ b/fs/afs/security.c
@@ -349,6 +349,16 @@ int afs_permission(struct inode *inode, int mask)
if (mask & MAY_NOT_BLOCK)
return -ECHILD;
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER) {
+ if (inode_owner_or_capable(inode))
+ return 0;
+ mask &= ~MAY_ACT_AS_OWNER;
+ if (!mask)
+ /* No other permission will suffice */
+ return -EACCES;
+ }
+
_enter("{{%x:%u},%lx},%x,",
vnode->fid.vid, vnode->fid.vnode, vnode->flags, mask);
diff --git a/fs/attr.c b/fs/attr.c
index d22e8187477f..c1160bd9416b 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -87,7 +87,7 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
/* Make sure a caller can chmod. */
if (ia_valid & ATTR_MODE) {
- if (!inode_owner_or_capable(inode))
+ if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
/* Also check the setgid bit! */
if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
@@ -98,7 +98,7 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
/* Check for setting the inode time. */
if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
- if (!inode_owner_or_capable(inode))
+ if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
}
@@ -246,11 +246,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
if (IS_IMMUTABLE(inode))
return -EPERM;
- if (!inode_owner_or_capable(inode)) {
- error = inode_permission(inode, MAY_WRITE);
- if (error)
- return error;
- }
+ error = inode_permission(inode, MAY_ACT_AS_OWNER | MAY_WRITE);
+ if (error)
+ return error;
}
if ((ia_valid & ATTR_MODE)) {
diff --git a/fs/coda/dir.c b/fs/coda/dir.c
index 00876ddadb43..7e31f68d4973 100644
--- a/fs/coda/dir.c
+++ b/fs/coda/dir.c
@@ -80,6 +80,16 @@ int coda_permission(struct inode *inode, int mask)
if (mask & MAY_NOT_BLOCK)
return -ECHILD;
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER) {
+ if (inode_owner_or_capable(inode))
+ return 0;
+ mask &= ~MAY_ACT_AS_OWNER;
+ if (!mask)
+ /* No other permission will suffice */
+ return -EACCES;
+ }
+
mask &= MAY_READ | MAY_WRITE | MAY_EXEC;
if (!mask)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 4137d96534a6..cc1d51150584 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -46,7 +46,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
/* O_NOATIME can only be set by the owner or superuser */
if ((arg & O_NOATIME) && !(filp->f_flags & O_NOATIME))
- if (!inode_owner_or_capable(inode))
+ if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
/* required for strict SunOS emulation */
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 0979609d6eba..3ff5a8f3a086 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1118,6 +1118,16 @@ static int fuse_permission(struct inode *inode, int mask)
if (!fuse_allow_current_process(fc))
return -EACCES;
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER) {
+ if (inode_owner_or_capable(inode))
+ return 0;
+ mask &= ~MAY_ACT_AS_OWNER;
+ if (!mask)
+ /* No other permission will suffice */
+ return -EACCES;
+ }
+
/*
* If attributes are needed, refresh them before proceeding
*/
diff --git a/fs/namei.c b/fs/namei.c
index 0cab6494978c..a033a0f5c284 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -335,6 +335,15 @@ int generic_permission(struct inode *inode, int mask)
{
int ret;
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER) {
+ if (inode_owner_or_capable(inode))
+ return 0;
+ mask &= ~MAY_ACT_AS_OWNER;
+ if (!mask)
+ /* No other permission will suffice */
+ return -EACCES;
+ }
/*
* Do the basic permission checks.
*/
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 8bfaa658b2c1..1fe54374b7c9 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2524,6 +2524,14 @@ int nfs_permission(struct inode *inode, int mask)
nfs_inc_stats(inode, NFSIOS_VFSACCESS);
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER)
+ /*
+ * Ownership will be tested by server when we
+ * actually try operation.
+ */
+ return 0;
+
if ((mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)
goto out;
/* Is this sys_access() ? */
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 2fd0fde16fe1..a90c7dca892a 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -863,7 +863,7 @@ set_posix_acl(struct inode *inode, int type, struct posix_acl *acl)
if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
return acl ? -EACCES : 0;
- if (!inode_owner_or_capable(inode))
+ if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
if (acl) {
diff --git a/fs/xattr.c b/fs/xattr.c
index daa732550088..78faa09a577b 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -126,7 +126,7 @@ xattr_permission(struct inode *inode, const char *name, int mask)
if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
return (mask & MAY_WRITE) ? -EPERM : -ENODATA;
if (S_ISDIR(inode->i_mode) && (inode->i_mode & S_ISVTX) &&
- (mask & MAY_WRITE) && !inode_owner_or_capable(inode))
+ (mask & MAY_WRITE) && inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 551cbc5574d7..5a8878a88cbb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -94,6 +94,14 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
#define MAY_CHDIR 0x00000040
/* called from RCU mode, don't block */
#define MAY_NOT_BLOCK 0x00000080
+/*
+ * File Owner is always allowed to perform pending
+ * operation. If current user is an owner, or if
+ * filesystem performs permission check at time-of-operation,
+ * then succeed, else require some other permission
+ * if listed.
+ */
+#define MAY_ACT_AS_OWNER 0x00000100
/*
* flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] VFS: allow MAY_ flags to be easily extended.
2018-10-04 1:02 [PATCH 0/3] Fix regression in NFSv3 ACL setting NeilBrown
2018-10-04 1:02 ` [PATCH 3/3] NFSD - Use MAY_ACT_AS_OWNER NeilBrown
2018-10-04 1:02 ` [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER NeilBrown
@ 2018-10-04 1:02 ` NeilBrown
2018-10-04 2:11 ` [PATCH 2/3 v2] " NeilBrown
2018-10-04 14:10 ` [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER David Howells
3 siblings, 1 reply; 10+ messages in thread
From: NeilBrown @ 2018-10-04 1:02 UTC (permalink / raw)
To: J. Bruce Fields, Anna Schumaker, Alexander Viro, Trond Myklebust
Cc: Jan Harkes, linux-nfs, Miklos Szeredi, Jeff Layton, linux-kernel,
linux-afs, David Howells, coda, linux-fsdevel, Christoph Hellwig
NFSD uses permission flags similar to the MAY_* flags,
with some overlap, and depends on the overlap matching.
This is currently a little fragile and hard to extend.
So add __MAY_UNUSED to identify the first unused flag,
and have NFSD use that flag and later flags for its
non-standard permissions.
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/nfsd/vfs.h | 33 ++++++++++++++++++---------------
include/linux/fs.h | 2 ++
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index a7e107309f76..2b1c70d3757a 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -13,23 +13,26 @@
* Flags for nfsd_permission
*/
#define NFSD_MAY_NOP 0
-#define NFSD_MAY_EXEC 0x001 /* == MAY_EXEC */
-#define NFSD_MAY_WRITE 0x002 /* == MAY_WRITE */
-#define NFSD_MAY_READ 0x004 /* == MAY_READ */
-#define NFSD_MAY_SATTR 0x008
-#define NFSD_MAY_TRUNC 0x010
-#define NFSD_MAY_LOCK 0x020
-#define NFSD_MAY_MASK 0x03f
+#define NFSD_MAY_EXEC MAY_EXEC
+#define NFSD_MAY_WRITE MAY_WRITE
+#define NFSD_MAY_READ MAY_READ
+#define NFSD_MAY_SATTR (__MAY_UNUSED << 0)
+#define NFSD_MAY_TRUNC (__MAY_UNUSED << 1)
+#define NFSD_MAY_LOCK (__MAY_UNUSED << 2)
+#define __NFSD_MAY_FIRST_HINT (__MAY_UNUSED << 3)
+#define NFSD_MAY_MASK (__NFSD_MAY_FIRST_HINT - 1)
/* extra hints to permission and open routines: */
-#define NFSD_MAY_OWNER_OVERRIDE 0x040
-#define NFSD_MAY_LOCAL_ACCESS 0x080 /* for device special files */
-#define NFSD_MAY_BYPASS_GSS_ON_ROOT 0x100
-#define NFSD_MAY_NOT_BREAK_LEASE 0x200
-#define NFSD_MAY_BYPASS_GSS 0x400
-#define NFSD_MAY_READ_IF_EXEC 0x800
-
-#define NFSD_MAY_64BIT_COOKIE 0x1000 /* 64 bit readdir cookies for >= NFSv3 */
+#define NFSD_MAY_OWNER_OVERRIDE (__NFSD_MAY_FIRST_HINT << 0)
+/* for device special files */
+#define NFSD_MAY_LOCAL_ACCESS (__NFSD_MAY_FIRST_HINT << 1)
+#define NFSD_MAY_BYPASS_GSS_ON_ROOT (__NFSD_MAY_FIRST_HINT << 2)
+#define NFSD_MAY_NOT_BREAK_LEASE (__NFSD_MAY_FIRST_HINT << 3)
+#define NFSD_MAY_BYPASS_GSS (__NFSD_MAY_FIRST_HINT << 4)
+#define NFSD_MAY_READ_IF_EXEC (__NFSD_MAY_FIRST_HINT << 5)
+
+/* 64 bit readdir cookies for >= NFSv3 */
+#define NFSD_MAY_64BIT_COOKIE (__NFSD_MAY_FIRST_HINT << 6)
#define NFSD_MAY_CREATE (NFSD_MAY_EXEC|NFSD_MAY_WRITE)
#define NFSD_MAY_REMOVE (NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5a8878a88cbb..b0076cb0f738 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -103,6 +103,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
*/
#define MAY_ACT_AS_OWNER 0x00000100
+#define __MAY_UNUSED 0x00000100
+
/*
* flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond
* to O_WRONLY and O_RDWR via the strange trick in do_dentry_open()
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER
2018-10-04 1:02 [PATCH 0/3] Fix regression in NFSv3 ACL setting NeilBrown
` (2 preceding siblings ...)
2018-10-04 1:02 ` [PATCH 2/3] VFS: allow MAY_ flags to be easily extended NeilBrown
@ 2018-10-04 14:10 ` David Howells
2018-10-04 14:42 ` Jan Harkes
` (2 more replies)
3 siblings, 3 replies; 10+ messages in thread
From: David Howells @ 2018-10-04 14:10 UTC (permalink / raw)
To: NeilBrown
Cc: dhowells, J. Bruce Fields, Anna Schumaker, Alexander Viro,
Trond Myklebust, Jan Harkes, linux-nfs, Miklos Szeredi,
Jeff Layton, linux-kernel, linux-afs, coda, linux-fsdevel,
Christoph Hellwig
NeilBrown <neilb@suse.com> wrote:
> diff --git a/fs/afs/security.c b/fs/afs/security.c
> index 81dfedb7879f..ac2e39de8bff 100644
> --- a/fs/afs/security.c
> +++ b/fs/afs/security.c
> @@ -349,6 +349,16 @@ int afs_permission(struct inode *inode, int mask)
> if (mask & MAY_NOT_BLOCK)
> return -ECHILD;
>
> + /* Short-circuit for owner */
> + if (mask & MAY_ACT_AS_OWNER) {
> + if (inode_owner_or_capable(inode))
You don't know that inode->i_uid in meaningful. You may have noticed that
afs_permission() ignores i_uid and i_gid entirely. It queries the server (if
this information is not otherwise cached) to ask what permits the user is
granted - where the user identity is defined by the key returned from
afs_request_key()[*].
So, NAK for the afs piece.
David
[*] If there's no appropriate key, anonymous permits will be used.
^ permalink raw reply [flat|nested] 10+ messages in thread