* [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
@ 2015-01-17 23:26 Ben Hutchings
2015-01-20 23:17 ` James Morris
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ben Hutchings @ 2015-01-17 23:26 UTC (permalink / raw)
To: Alexander Viro
Cc: linux-fsdevel, linux-security-module, LKML, 770492, Ben Harris,
oss-security
[-- Attachment #1: Type: text/plain, Size: 22950 bytes --]
chown() and write() should clear all privilege attributes on
a file - setuid, setgid, setcap and any other extended
privilege attributes.
However, any attributes beyond setuid and setgid are managed by the
LSM and not directly by the filesystem, so they cannot be set along
with the other attributes.
Currently we call security_inode_killpriv() in notify_change(),
but in case of a chown() this is too early - we have not called
inode_change_ok() or made any filesystem-specific permission/sanity
checks.
Add a new function setattr_killpriv() which calls
security_inode_killpriv() if necessary, and change the setattr()
implementation to call this in each filesystem that supports xattrs.
This assumes that extended privilege attributes are always stored in
xattrs.
Compile-tested only.
XXX This is a silent change to the VFS API, but we should probably
change something so OOT filesystems fail to compile if they aren't
updated to call setattr_killpriv().
Reported-by: Ben Harris <bjh21@cam.ac.uk>
References: https://bugs.debian.org/770492
---
drivers/staging/lustre/lustre/llite/llite_lib.c | 4 ++++
fs/9p/vfs_inode.c | 4 ++++
fs/9p/vfs_inode_dotl.c | 4 ++++
fs/attr.c | 32 +++++++++++++++++++++----
fs/btrfs/inode.c | 4 ++++
fs/ceph/inode.c | 4 ++++
fs/cifs/inode.c | 11 ++++++++-
fs/ext2/inode.c | 4 ++++
fs/ext3/inode.c | 4 ++++
fs/ext4/inode.c | 4 ++++
fs/f2fs/file.c | 4 ++++
fs/fuse/dir.c | 15 +++++++-----
fs/fuse/file.c | 3 ++-
fs/fuse/fuse_i.h | 2 +-
fs/gfs2/inode.c | 3 +++
fs/hfs/inode.c | 4 ++++
fs/hfsplus/inode.c | 4 ++++
fs/jffs2/fs.c | 4 ++++
fs/jfs/file.c | 4 ++++
fs/kernfs/inode.c | 17 +++++++++++++
fs/libfs.c | 3 +++
fs/nfs/inode.c | 11 +++++++--
fs/ocfs2/file.c | 6 ++++-
fs/reiserfs/inode.c | 4 ++++
fs/ubifs/file.c | 4 ++++
fs/xfs/xfs_acl.c | 3 ++-
fs/xfs/xfs_file.c | 2 +-
fs/xfs/xfs_ioctl.c | 2 +-
fs/xfs/xfs_iops.c | 16 ++++++++++---
fs/xfs/xfs_iops.h | 10 ++++++--
include/linux/fs.h | 1 +
mm/shmem.c | 4 ++++
32 files changed, 176 insertions(+), 25 deletions(-)
diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
index a8bcc51..2a714b2 100644
--- a/drivers/staging/lustre/lustre/llite/llite_lib.c
+++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
@@ -1434,6 +1434,10 @@ int ll_setattr_raw(struct dentry *dentry, struct iattr *attr, bool hsm_import)
spin_unlock(&lli->lli_lock);
}
+ rc = setattr_killpriv(dentry, attr);
+ if (rc)
+ return rc;
+
/* We always do an MDS RPC, even if we're only changing the size;
* only the MDS knows whether truncate() should fail with -ETXTBUSY */
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 296482f..735cbf84 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1130,6 +1130,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
if (S_ISREG(dentry->d_inode->i_mode))
filemap_write_and_wait(dentry->d_inode->i_mapping);
+ retval = setattr_killpriv(dentry, iattr);
+ if (retval)
+ return retval;
+
retval = p9_client_wstat(fid, &wstat);
if (retval < 0)
return retval;
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 02b64f4..f3ca76d 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -583,6 +583,10 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr)
if (S_ISREG(inode->i_mode))
filemap_write_and_wait(inode->i_mapping);
+ retval = setattr_killpriv(dentry, iattr);
+ if (retval)
+ return retval;
+
retval = p9_client_setattr(fid, &p9attr);
if (retval < 0)
return retval;
diff --git a/fs/attr.c b/fs/attr.c
index 6530ced..184f3bf 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -168,6 +168,28 @@ void setattr_copy(struct inode *inode, const struct iattr *attr)
EXPORT_SYMBOL(setattr_copy);
/**
+ * setattr_killpriv - remove extended privilege attributes from a file
+ * @dentry: Directory entry passed to the setattr operation
+ * @iattr: New attributes pased to the setattr operation
+ *
+ * All filesystems that can carry extended privilege attributes
+ * should call this from their setattr operation *after* validating
+ * the attribute changes.
+ *
+ * It does nothing if !(iattr->ia_valid & ATTR_KILL_PRIV), so
+ * it is not necessary to call it in that case.
+ */
+int setattr_killpriv(struct dentry *dentry, struct iattr *iattr)
+{
+ if (!(iattr->ia_valid & ATTR_KILL_PRIV))
+ return 0;
+
+ iattr->ia_valid &= ~ATTR_KILL_PRIV;
+ return security_inode_killpriv(dentry);
+}
+EXPORT_SYMBOL(setattr_killpriv);
+
+/**
* notify_change - modify attributes of a filesytem object
* @dentry: object affected
* @iattr: new attributes
@@ -217,13 +239,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
if (!(ia_valid & ATTR_MTIME_SET))
attr->ia_mtime = now;
if (ia_valid & ATTR_KILL_PRIV) {
- attr->ia_valid &= ~ATTR_KILL_PRIV;
- ia_valid &= ~ATTR_KILL_PRIV;
error = security_inode_need_killpriv(dentry);
- if (error > 0)
- error = security_inode_killpriv(dentry);
- if (error)
+ if (error < 0)
return error;
+ if (error == 0) {
+ attr->ia_valid &= ~ATTR_KILL_PRIV;
+ ia_valid &= ~ATTR_KILL_PRIV;
+ }
}
/*
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d23362f..71e3fb8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4697,6 +4697,10 @@ static int btrfs_setattr(struct dentry *dentry, struct iattr *attr)
if (err)
return err;
+ err = setattr_killpriv(dentry, attr);
+ if (err)
+ return err;
+
if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
err = btrfs_setsize(inode, attr);
if (err)
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 7b61390..9ba5556 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1712,6 +1712,10 @@ int ceph_setattr(struct dentry *dentry, struct iattr *attr)
if (err != 0)
return err;
+ err = setattr_killpriv(dentry, attr);
+ if (err != 0)
+ return err;
+
req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_SETATTR,
USE_AUTH_MDS);
if (IS_ERR(req))
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 197cb50..0e971f9 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2149,7 +2149,10 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
*/
rc = filemap_write_and_wait(inode->i_mapping);
mapping_set_error(inode->i_mapping, rc);
- rc = 0;
+
+ rc = setattr_killpriv(direntry, attrs);
+ if (rc)
+ goto out;
if (attrs->ia_valid & ATTR_SIZE) {
rc = cifs_set_file_size(inode, attrs, xid, full_path);
@@ -2273,6 +2276,12 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
return rc;
}
+ rc = setattr_killpriv(direntry, attrs);
+ if (rc) {
+ free_xid(xid);
+ return rc;
+ }
+
full_path = build_path_from_dentry(direntry);
if (full_path == NULL) {
rc = -ENOMEM;
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 36d35c3..9e245af 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1551,6 +1551,10 @@ int ext2_setattr(struct dentry *dentry, struct iattr *iattr)
if (error)
return error;
+ error = setattr_killpriv(dentry, iattr);
+ if (error)
+ return error;
+
if (is_quota_modification(inode, iattr))
dquot_initialize(inode);
if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 2c6ccc4..ec4dffa 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -3248,6 +3248,10 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
if (error)
return error;
+ error = setattr_killpriv(dentry, attr);
+ if (error)
+ return error;
+
if (is_quota_modification(inode, attr))
dquot_initialize(inode);
if ((ia_valid & ATTR_UID && !uid_eq(attr->ia_uid, inode->i_uid)) ||
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3356ab5..80877a48 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4455,6 +4455,10 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
if (error)
return error;
+ error = setattr_killpriv(dentry, attr);
+ if (error)
+ return error;
+
if (is_quota_modification(inode, attr))
dquot_initialize(inode);
if ((ia_valid & ATTR_UID && !uid_eq(attr->ia_uid, inode->i_uid)) ||
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 8e68bb6..c9371d2 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -560,6 +560,10 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
if (err)
return err;
+ err = setattr_killpriv(dentry, attr);
+ if (err)
+ return err;
+
if (attr->ia_valid & ATTR_SIZE) {
err = f2fs_convert_inline_data(inode, attr->ia_size, NULL);
if (err)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index dbab798..f750848 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1693,9 +1693,10 @@ int fuse_flush_times(struct inode *inode, struct fuse_file *ff)
* vmtruncate() doesn't allow for this case, so do the rlimit checking
* and the actual truncation by hand.
*/
-int fuse_do_setattr(struct inode *inode, struct iattr *attr,
+int fuse_do_setattr(struct dentry *entry, struct iattr *attr,
struct file *file)
{
+ struct inode *inode = entry->d_inode;
struct fuse_conn *fc = get_fuse_conn(inode);
struct fuse_inode *fi = get_fuse_inode(inode);
struct fuse_req *req;
@@ -1714,6 +1715,10 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
if (err)
return err;
+ err = setattr_killpriv(entry, attr);
+ if (err)
+ return err;
+
if (attr->ia_valid & ATTR_OPEN) {
if (fc->atomic_o_trunc)
return 0;
@@ -1809,15 +1814,13 @@ error:
static int fuse_setattr(struct dentry *entry, struct iattr *attr)
{
- struct inode *inode = entry->d_inode;
-
- if (!fuse_allow_current_process(get_fuse_conn(inode)))
+ if (!fuse_allow_current_process(get_fuse_conn(entry->d_inode)))
return -EACCES;
if (attr->ia_valid & ATTR_FILE)
- return fuse_do_setattr(inode, attr, attr->ia_file);
+ return fuse_do_setattr(entry, attr, attr->ia_file);
else
- return fuse_do_setattr(inode, attr, NULL);
+ return fuse_do_setattr(entry, attr, NULL);
}
static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index caa8d95..ffdc363 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2855,7 +2855,8 @@ static void fuse_do_truncate(struct file *file)
attr.ia_file = file;
attr.ia_valid |= ATTR_FILE;
- fuse_do_setattr(inode, &attr, file);
+ /* XXX Is file->f_dentry->d_inode always the same as inode? */
+ fuse_do_setattr(file->f_dentry, &attr, file);
}
static inline loff_t fuse_round_up(loff_t off)
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index e8e47a6..163de1f 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -894,7 +894,7 @@ bool fuse_write_update_size(struct inode *inode, loff_t pos);
int fuse_flush_times(struct inode *inode, struct fuse_file *ff);
int fuse_write_inode(struct inode *inode, struct writeback_control *wbc);
-int fuse_do_setattr(struct inode *inode, struct iattr *attr,
+int fuse_do_setattr(struct dentry *entry, struct iattr *attr,
struct file *file);
#endif /* _FS_FUSE_I_H */
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index c4ed823..b39d81a 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -1786,6 +1786,9 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr)
if (error)
goto out;
+ error = setattr_killpriv(dentry, attr);
+ if (error)
+ goto out;
if (attr->ia_valid & ATTR_SIZE)
error = gfs2_setattr_size(inode, attr->ia_size);
else if (attr->ia_valid & (ATTR_UID | ATTR_GID))
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index d0929bc..817f7a5 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -620,6 +620,10 @@ int hfs_inode_setattr(struct dentry *dentry, struct iattr * attr)
return hsb->s_quiet ? 0 : error;
}
+ error = setattr_killpriv(dentry, attr);
+ if (error)
+ return error;
+
if (attr->ia_valid & ATTR_MODE) {
/* Only the 'w' bits can ever change and only all together. */
if (attr->ia_mode & S_IWUSR)
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 0cf786f..12549bc 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -251,6 +251,10 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
if (error)
return error;
+ error = setattr_killpriv(dentry, attr);
+ if (error)
+ return error;
+
if ((attr->ia_valid & ATTR_SIZE) &&
attr->ia_size != i_size_read(inode)) {
inode_dio_wait(inode);
diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 601afd1..b260789 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -197,6 +197,10 @@ int jffs2_setattr(struct dentry *dentry, struct iattr *iattr)
if (rc)
return rc;
+ rc = setattr_killpriv(dentry, iattr);
+ if (rc)
+ return rc;
+
rc = jffs2_do_setattr(inode, iattr);
if (!rc && (iattr->ia_valid & ATTR_MODE))
rc = posix_acl_chmod(inode, inode->i_mode);
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 33aa0cc..4008313 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -107,6 +107,10 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr)
if (rc)
return rc;
+ rc = setattr_killpriv(dentry, iattr);
+ if (rc)
+ return rc;
+
if (is_quota_modification(inode, iattr))
dquot_initialize(inode);
if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 9852176..6a70fc5 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -135,6 +135,23 @@ int kernfs_iop_setattr(struct dentry *dentry, struct iattr *iattr)
if (error)
goto out;
+ /*
+ * If we need to remove privileges, drop the mutex to do that
+ * first and then re-validate the remaining changes.
+ */
+ if (iattr->ia_valid & ATTR_KILL_PRIV) {
+ mutex_unlock(&kernfs_mutex);
+
+ error = setattr_killpriv(dentry, iattr);
+ if (error)
+ return error;
+
+ mutex_lock(&kernfs_mutex);
+ error = inode_change_ok(inode, iattr);
+ if (error)
+ goto out;
+ }
+
error = __kernfs_setattr(kn, iattr);
if (error)
goto out;
diff --git a/fs/libfs.c b/fs/libfs.c
index 171d284..9a00049 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -375,6 +375,9 @@ int simple_setattr(struct dentry *dentry, struct iattr *iattr)
if (error)
return error;
+ error = setattr_killpriv(dentry, iattr);
+ if (error)
+ return error;
if (iattr->ia_valid & ATTR_SIZE)
truncate_setsize(inode, iattr->ia_size);
setattr_copy(inode, iattr);
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 00689a8..94dd6ac 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -496,7 +496,7 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
{
struct inode *inode = dentry->d_inode;
struct nfs_fattr *fattr;
- int error = -ENOMEM;
+ int error;
nfs_inc_stats(inode, NFSIOS_VFSSETATTR);
@@ -524,9 +524,16 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
nfs_wb_all(inode);
}
+ /* XXX Can we assume the server's permission checks are sufficient? */
+ error = setattr_killpriv(dentry, attr);
+ if (error)
+ goto out;
+
fattr = nfs_alloc_fattr();
- if (fattr == NULL)
+ if (fattr == NULL) {
+ error = -ENOMEM;
goto out;
+ }
/*
* Return any delegations if we're going to change ACLs
*/
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 324dc93..ed93d74 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1139,7 +1139,7 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
attr->ia_valid &= ~ATTR_SIZE;
#define OCFS2_VALID_ATTRS (ATTR_ATIME | ATTR_MTIME | ATTR_CTIME | ATTR_SIZE \
- | ATTR_GID | ATTR_UID | ATTR_MODE)
+ | ATTR_GID | ATTR_UID | ATTR_MODE | ATTR_KILL_PRIV)
if (!(attr->ia_valid & OCFS2_VALID_ATTRS))
return 0;
@@ -1147,6 +1147,10 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
if (status)
return status;
+ status = setattr_killpriv(dentry, attr);
+ if (status)
+ return status;
+
if (is_quota_modification(inode, attr))
dquot_initialize(inode);
size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE;
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index a7eec98..a458c12 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -3316,6 +3316,10 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
if (error)
return error;
+ error = setattr_killpriv(dentry, attr);
+ if (error)
+ return error;
+
/* must be turned off for recursive notify_change calls */
ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index b5b593c..73d2e87 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1269,6 +1269,10 @@ int ubifs_setattr(struct dentry *dentry, struct iattr *attr)
if (err)
return err;
+ err = setattr_killpriv(dentry, attr);
+ if (err)
+ return err;
+
if ((attr->ia_valid & ATTR_SIZE) && attr->ia_size < inode->i_size)
/* Truncation to a smaller size */
err = do_truncation(c, inode, attr);
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index a65fa5d..22b7482 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -244,7 +244,8 @@ xfs_set_mode(struct inode *inode, umode_t mode)
iattr.ia_mode = mode;
iattr.ia_ctime = current_fs_time(inode->i_sb);
- error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
+ error = xfs_setattr_nonsize(NULL, XFS_I(inode), &iattr,
+ XFS_ATTR_NOACL);
}
return error;
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index eb596b4..c9b9019 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -873,7 +873,7 @@ xfs_file_fallocate(
iattr.ia_valid = ATTR_SIZE;
iattr.ia_size = new_size;
- error = xfs_setattr_size(ip, &iattr);
+ error = xfs_setattr_size(NULL, ip, &iattr);
}
out_unlock:
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 24c926b6..3e0dc56 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -714,7 +714,7 @@ xfs_ioc_space(
iattr.ia_valid = ATTR_SIZE;
iattr.ia_size = bf->l_start;
- error = xfs_setattr_size(ip, &iattr);
+ error = xfs_setattr_size(NULL, ip, &iattr);
if (!error)
clrprealloc = true;
break;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ec6dcdc..669150b 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -527,6 +527,7 @@ xfs_setattr_time(
int
xfs_setattr_nonsize(
+ struct dentry *dentry,
struct xfs_inode *ip,
struct iattr *iattr,
int flags)
@@ -554,6 +555,10 @@ xfs_setattr_nonsize(
error = inode_change_ok(inode, iattr);
if (error)
return error;
+
+ error = setattr_killpriv(dentry, iattr);
+ if (error)
+ return error;
}
ASSERT((mask & ATTR_SIZE) == 0);
@@ -734,6 +739,7 @@ out_dqrele:
*/
int
xfs_setattr_size(
+ struct dentry *dentry,
struct xfs_inode *ip,
struct iattr *iattr)
{
@@ -776,9 +782,13 @@ xfs_setattr_size(
* Use the regular setattr path to update the timestamps.
*/
iattr->ia_valid &= ~ATTR_SIZE;
- return xfs_setattr_nonsize(ip, iattr, 0);
+ return xfs_setattr_nonsize(dentry, ip, iattr, 0);
}
+ error = setattr_killpriv(dentry, iattr);
+ if (error)
+ return error;
+
/*
* Make sure that the dquots are attached to the inode.
*/
@@ -974,10 +984,10 @@ xfs_vn_setattr(
if (iattr->ia_valid & ATTR_SIZE) {
xfs_ilock(ip, XFS_IOLOCK_EXCL);
- error = xfs_setattr_size(ip, iattr);
+ error = xfs_setattr_size(dentry, ip, iattr);
xfs_iunlock(ip, XFS_IOLOCK_EXCL);
} else {
- error = xfs_setattr_nonsize(ip, iattr, 0);
+ error = xfs_setattr_nonsize(dentry, ip, iattr, 0);
}
return error;
diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
index 1c34e43..6994d3e 100644
--- a/fs/xfs/xfs_iops.h
+++ b/fs/xfs/xfs_iops.h
@@ -32,8 +32,14 @@ extern void xfs_setup_inode(struct xfs_inode *);
*/
#define XFS_ATTR_NOACL 0x01 /* Don't call posix_acl_chmod */
-extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
+/*
+ * XXX Several callers have to pass dentry = NULL and this should
+ * work but it's really ugly.
+ */
+extern int xfs_setattr_nonsize(struct dentry *dentry,
+ struct xfs_inode *ip, struct iattr *vap,
int flags);
-extern int xfs_setattr_size(struct xfs_inode *ip, struct iattr *vap);
+extern int xfs_setattr_size(struct dentry *dentry,
+ struct xfs_inode *ip, struct iattr *vap);
#endif /* __XFS_IOPS_H__ */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9ab779e..7cad5d1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2663,6 +2663,7 @@ extern int buffer_migrate_page(struct address_space *,
extern int inode_change_ok(const struct inode *, struct iattr *);
extern int inode_newsize_ok(const struct inode *, loff_t offset);
extern void setattr_copy(struct inode *inode, const struct iattr *attr);
+extern int setattr_killpriv(struct dentry *dentry, struct iattr *attr);
extern int file_update_time(struct file *file);
diff --git a/mm/shmem.c b/mm/shmem.c
index 185836b..d1d4b9b 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -557,6 +557,10 @@ static int shmem_setattr(struct dentry *dentry, struct iattr *attr)
if (error)
return error;
+ error = setattr_killpriv(dentry, attr);
+ if (error)
+ return error;
+
if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
loff_t oldsize = inode->i_size;
loff_t newsize = attr->ia_size;
--
Ben Hutchings
The first rule of tautology club is the first rule of tautology club.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
2015-01-17 23:26 [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks Ben Hutchings
@ 2015-01-20 23:17 ` James Morris
2015-01-20 23:32 ` Casey Schaufler
2015-01-21 14:03 ` Stephen Smalley
2015-02-16 19:50 ` Josh Boyer
2015-04-08 21:43 ` Mateusz Guzik
2 siblings, 2 replies; 9+ messages in thread
From: James Morris @ 2015-01-20 23:17 UTC (permalink / raw)
To: Ben Hutchings
Cc: Alexander Viro, linux-fsdevel, linux-security-module, LKML,
770492, Ben Harris, oss-security, John Johansen, Paul Moore,
Stephen Smalley, Casey Schaufler
On Sat, 17 Jan 2015, Ben Hutchings wrote:
> chown() and write() should clear all privilege attributes on
> a file - setuid, setgid, setcap and any other extended
> privilege attributes.
>
> However, any attributes beyond setuid and setgid are managed by the
> LSM and not directly by the filesystem, so they cannot be set along
> with the other attributes.
>
> Currently we call security_inode_killpriv() in notify_change(),
> but in case of a chown() this is too early - we have not called
> inode_change_ok() or made any filesystem-specific permission/sanity
> checks.
>
> Add a new function setattr_killpriv() which calls
> security_inode_killpriv() if necessary, and change the setattr()
> implementation to call this in each filesystem that supports xattrs.
> This assumes that extended privilege attributes are always stored in
> xattrs.
It'd be useful to get some input from LSM module maintainers on this.
e.g. doesn't SELinux already handle this via policy directives?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
2015-01-20 23:17 ` James Morris
@ 2015-01-20 23:32 ` Casey Schaufler
2015-01-21 14:03 ` Stephen Smalley
1 sibling, 0 replies; 9+ messages in thread
From: Casey Schaufler @ 2015-01-20 23:32 UTC (permalink / raw)
To: James Morris, Ben Hutchings
Cc: Alexander Viro, linux-fsdevel, linux-security-module, LKML,
770492, Ben Harris, oss-security, John Johansen, Paul Moore,
Stephen Smalley, Casey Schaufler
On 1/20/2015 3:17 PM, James Morris wrote:
> On Sat, 17 Jan 2015, Ben Hutchings wrote:
>
>> chown() and write() should clear all privilege attributes on
>> a file - setuid, setgid, setcap and any other extended
>> privilege attributes.
>>
>> However, any attributes beyond setuid and setgid are managed by the
>> LSM and not directly by the filesystem, so they cannot be set along
>> with the other attributes.
>>
>> Currently we call security_inode_killpriv() in notify_change(),
>> but in case of a chown() this is too early - we have not called
>> inode_change_ok() or made any filesystem-specific permission/sanity
>> checks.
>>
>> Add a new function setattr_killpriv() which calls
>> security_inode_killpriv() if necessary, and change the setattr()
>> implementation to call this in each filesystem that supports xattrs.
>> This assumes that extended privilege attributes are always stored in
>> xattrs.
> It'd be useful to get some input from LSM module maintainers on this.
I've already chimed in.
Clearing the Smack label on a file because someone writes to it
makes no sense whatsoever. The same with chown. The Smack label is
attached to the object, which is a container of data, not the data
itself. Smack labels are Mandatory Access Control labels, not Information
labels. If that doesn't mean anything to the reader, check out the
P1003.1e/2c (withdrawn) DRAFT.
The proposed implementation does not correctly handle either
Mandatory Access Control labels or Information labels. The MAC
label is *very different* from the setuid bit.
>
> e.g. doesn't SELinux already handle this via policy directives?
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
2015-01-20 23:17 ` James Morris
2015-01-20 23:32 ` Casey Schaufler
@ 2015-01-21 14:03 ` Stephen Smalley
2015-01-21 16:15 ` Casey Schaufler
1 sibling, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2015-01-21 14:03 UTC (permalink / raw)
To: James Morris, Ben Hutchings
Cc: Alexander Viro, linux-fsdevel, linux-security-module, LKML,
770492, Ben Harris, oss-security, John Johansen, Paul Moore,
Casey Schaufler
On 01/20/2015 06:17 PM, James Morris wrote:
> On Sat, 17 Jan 2015, Ben Hutchings wrote:
>
>> chown() and write() should clear all privilege attributes on
>> a file - setuid, setgid, setcap and any other extended
>> privilege attributes.
>>
>> However, any attributes beyond setuid and setgid are managed by the
>> LSM and not directly by the filesystem, so they cannot be set along
>> with the other attributes.
>>
>> Currently we call security_inode_killpriv() in notify_change(),
>> but in case of a chown() this is too early - we have not called
>> inode_change_ok() or made any filesystem-specific permission/sanity
>> checks.
>>
>> Add a new function setattr_killpriv() which calls
>> security_inode_killpriv() if necessary, and change the setattr()
>> implementation to call this in each filesystem that supports xattrs.
>> This assumes that extended privilege attributes are always stored in
>> xattrs.
>
> It'd be useful to get some input from LSM module maintainers on this.
>
> e.g. doesn't SELinux already handle this via policy directives?
There have been a couple postings of a similar patch set [1] by Jan
Kara, although I don't believe that series addressed chown().
If I am reading the patches correctly, they (correctly) don't affect
SELinux or Smack labels; they are just calling the existing
security_inode_killpriv() hook, which is only implemented for the
capability module to remove the security.capability xattr.
[1] http://marc.info/?l=linux-security-module&m=141890696325054&w=2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
2015-01-21 14:03 ` Stephen Smalley
@ 2015-01-21 16:15 ` Casey Schaufler
0 siblings, 0 replies; 9+ messages in thread
From: Casey Schaufler @ 2015-01-21 16:15 UTC (permalink / raw)
To: Stephen Smalley, James Morris, Ben Hutchings
Cc: Alexander Viro, linux-fsdevel, linux-security-module, LKML,
770492, Ben Harris, oss-security, John Johansen, Paul Moore,
Casey Schaufler
On 1/21/2015 6:03 AM, Stephen Smalley wrote:
> On 01/20/2015 06:17 PM, James Morris wrote:
>> On Sat, 17 Jan 2015, Ben Hutchings wrote:
>>
>>> chown() and write() should clear all privilege attributes on
>>> a file - setuid, setgid, setcap and any other extended
>>> privilege attributes.
>>>
>>> However, any attributes beyond setuid and setgid are managed by the
>>> LSM and not directly by the filesystem, so they cannot be set along
>>> with the other attributes.
>>>
>>> Currently we call security_inode_killpriv() in notify_change(),
>>> but in case of a chown() this is too early - we have not called
>>> inode_change_ok() or made any filesystem-specific permission/sanity
>>> checks.
>>>
>>> Add a new function setattr_killpriv() which calls
>>> security_inode_killpriv() if necessary, and change the setattr()
>>> implementation to call this in each filesystem that supports xattrs.
>>> This assumes that extended privilege attributes are always stored in
>>> xattrs.
>> It'd be useful to get some input from LSM module maintainers on this.
>>
>> e.g. doesn't SELinux already handle this via policy directives?
> There have been a couple postings of a similar patch set [1] by Jan
> Kara, although I don't believe that series addressed chown().
>
> If I am reading the patches correctly, they (correctly) don't affect
> SELinux or Smack labels; they are just calling the existing
> security_inode_killpriv() hook, which is only implemented for the
> capability module to remove the security.capability xattr.
The description of the change should say that. I can easily
imagine an enthusiastic test developer reading the existing
description and filing bugs because SELinux, Smack and whatever
other xattr based systems might be around don't clear their
attributes. If the intent wasn't clear to the first person to
use xattrs for security purposes, I shouldn't expect the new
and inexperienced to see it.
My position softens. Document it correctly, and I'm fine with it.
>
> [1] http://marc.info/?l=linux-security-module&m=141890696325054&w=2
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
2015-01-17 23:26 [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks Ben Hutchings
2015-01-20 23:17 ` James Morris
@ 2015-02-16 19:50 ` Josh Boyer
2015-04-08 21:43 ` Mateusz Guzik
2 siblings, 0 replies; 9+ messages in thread
From: Josh Boyer @ 2015-02-16 19:50 UTC (permalink / raw)
To: Ben Hutchings
Cc: Alexander Viro, linux-fsdevel, linux-security-module, LKML,
770492, Ben Harris, oss-security
On Sat, Jan 17, 2015 at 6:26 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> chown() and write() should clear all privilege attributes on
> a file - setuid, setgid, setcap and any other extended
> privilege attributes.
>
> However, any attributes beyond setuid and setgid are managed by the
> LSM and not directly by the filesystem, so they cannot be set along
> with the other attributes.
>
> Currently we call security_inode_killpriv() in notify_change(),
> but in case of a chown() this is too early - we have not called
> inode_change_ok() or made any filesystem-specific permission/sanity
> checks.
>
> Add a new function setattr_killpriv() which calls
> security_inode_killpriv() if necessary, and change the setattr()
> implementation to call this in each filesystem that supports xattrs.
> This assumes that extended privilege attributes are always stored in
> xattrs.
>
> Compile-tested only.
>
> XXX This is a silent change to the VFS API, but we should probably
> change something so OOT filesystems fail to compile if they aren't
> updated to call setattr_killpriv().
>
> Reported-by: Ben Harris <bjh21@cam.ac.uk>
> References: https://bugs.debian.org/770492
This seems to have stalled. I don't see it in linux-next or anywhere
else I can find. The issue has a shiny CVE now, so it makes people
that follow those nervous. Is there any further feedback or follow-up
here?
josh
> ---
> drivers/staging/lustre/lustre/llite/llite_lib.c | 4 ++++
> fs/9p/vfs_inode.c | 4 ++++
> fs/9p/vfs_inode_dotl.c | 4 ++++
> fs/attr.c | 32 +++++++++++++++++++++----
> fs/btrfs/inode.c | 4 ++++
> fs/ceph/inode.c | 4 ++++
> fs/cifs/inode.c | 11 ++++++++-
> fs/ext2/inode.c | 4 ++++
> fs/ext3/inode.c | 4 ++++
> fs/ext4/inode.c | 4 ++++
> fs/f2fs/file.c | 4 ++++
> fs/fuse/dir.c | 15 +++++++-----
> fs/fuse/file.c | 3 ++-
> fs/fuse/fuse_i.h | 2 +-
> fs/gfs2/inode.c | 3 +++
> fs/hfs/inode.c | 4 ++++
> fs/hfsplus/inode.c | 4 ++++
> fs/jffs2/fs.c | 4 ++++
> fs/jfs/file.c | 4 ++++
> fs/kernfs/inode.c | 17 +++++++++++++
> fs/libfs.c | 3 +++
> fs/nfs/inode.c | 11 +++++++--
> fs/ocfs2/file.c | 6 ++++-
> fs/reiserfs/inode.c | 4 ++++
> fs/ubifs/file.c | 4 ++++
> fs/xfs/xfs_acl.c | 3 ++-
> fs/xfs/xfs_file.c | 2 +-
> fs/xfs/xfs_ioctl.c | 2 +-
> fs/xfs/xfs_iops.c | 16 ++++++++++---
> fs/xfs/xfs_iops.h | 10 ++++++--
> include/linux/fs.h | 1 +
> mm/shmem.c | 4 ++++
> 32 files changed, 176 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
> index a8bcc51..2a714b2 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_lib.c
> +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
> @@ -1434,6 +1434,10 @@ int ll_setattr_raw(struct dentry *dentry, struct iattr *attr, bool hsm_import)
> spin_unlock(&lli->lli_lock);
> }
>
> + rc = setattr_killpriv(dentry, attr);
> + if (rc)
> + return rc;
> +
> /* We always do an MDS RPC, even if we're only changing the size;
> * only the MDS knows whether truncate() should fail with -ETXTBUSY */
>
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 296482f..735cbf84 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -1130,6 +1130,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
> if (S_ISREG(dentry->d_inode->i_mode))
> filemap_write_and_wait(dentry->d_inode->i_mapping);
>
> + retval = setattr_killpriv(dentry, iattr);
> + if (retval)
> + return retval;
> +
> retval = p9_client_wstat(fid, &wstat);
> if (retval < 0)
> return retval;
> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 02b64f4..f3ca76d 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -583,6 +583,10 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr)
> if (S_ISREG(inode->i_mode))
> filemap_write_and_wait(inode->i_mapping);
>
> + retval = setattr_killpriv(dentry, iattr);
> + if (retval)
> + return retval;
> +
> retval = p9_client_setattr(fid, &p9attr);
> if (retval < 0)
> return retval;
> diff --git a/fs/attr.c b/fs/attr.c
> index 6530ced..184f3bf 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -168,6 +168,28 @@ void setattr_copy(struct inode *inode, const struct iattr *attr)
> EXPORT_SYMBOL(setattr_copy);
>
> /**
> + * setattr_killpriv - remove extended privilege attributes from a file
> + * @dentry: Directory entry passed to the setattr operation
> + * @iattr: New attributes pased to the setattr operation
> + *
> + * All filesystems that can carry extended privilege attributes
> + * should call this from their setattr operation *after* validating
> + * the attribute changes.
> + *
> + * It does nothing if !(iattr->ia_valid & ATTR_KILL_PRIV), so
> + * it is not necessary to call it in that case.
> + */
> +int setattr_killpriv(struct dentry *dentry, struct iattr *iattr)
> +{
> + if (!(iattr->ia_valid & ATTR_KILL_PRIV))
> + return 0;
> +
> + iattr->ia_valid &= ~ATTR_KILL_PRIV;
> + return security_inode_killpriv(dentry);
> +}
> +EXPORT_SYMBOL(setattr_killpriv);
> +
> +/**
> * notify_change - modify attributes of a filesytem object
> * @dentry: object affected
> * @iattr: new attributes
> @@ -217,13 +239,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
> if (!(ia_valid & ATTR_MTIME_SET))
> attr->ia_mtime = now;
> if (ia_valid & ATTR_KILL_PRIV) {
> - attr->ia_valid &= ~ATTR_KILL_PRIV;
> - ia_valid &= ~ATTR_KILL_PRIV;
> error = security_inode_need_killpriv(dentry);
> - if (error > 0)
> - error = security_inode_killpriv(dentry);
> - if (error)
> + if (error < 0)
> return error;
> + if (error == 0) {
> + attr->ia_valid &= ~ATTR_KILL_PRIV;
> + ia_valid &= ~ATTR_KILL_PRIV;
> + }
> }
>
> /*
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d23362f..71e3fb8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4697,6 +4697,10 @@ static int btrfs_setattr(struct dentry *dentry, struct iattr *attr)
> if (err)
> return err;
>
> + err = setattr_killpriv(dentry, attr);
> + if (err)
> + return err;
> +
> if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
> err = btrfs_setsize(inode, attr);
> if (err)
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 7b61390..9ba5556 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1712,6 +1712,10 @@ int ceph_setattr(struct dentry *dentry, struct iattr *attr)
> if (err != 0)
> return err;
>
> + err = setattr_killpriv(dentry, attr);
> + if (err != 0)
> + return err;
> +
> req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_SETATTR,
> USE_AUTH_MDS);
> if (IS_ERR(req))
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 197cb50..0e971f9 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -2149,7 +2149,10 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
> */
> rc = filemap_write_and_wait(inode->i_mapping);
> mapping_set_error(inode->i_mapping, rc);
> - rc = 0;
> +
> + rc = setattr_killpriv(direntry, attrs);
> + if (rc)
> + goto out;
>
> if (attrs->ia_valid & ATTR_SIZE) {
> rc = cifs_set_file_size(inode, attrs, xid, full_path);
> @@ -2273,6 +2276,12 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
> return rc;
> }
>
> + rc = setattr_killpriv(direntry, attrs);
> + if (rc) {
> + free_xid(xid);
> + return rc;
> + }
> +
> full_path = build_path_from_dentry(direntry);
> if (full_path == NULL) {
> rc = -ENOMEM;
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 36d35c3..9e245af 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1551,6 +1551,10 @@ int ext2_setattr(struct dentry *dentry, struct iattr *iattr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> +
> if (is_quota_modification(inode, iattr))
> dquot_initialize(inode);
> if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index 2c6ccc4..ec4dffa 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -3248,6 +3248,10 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if (is_quota_modification(inode, attr))
> dquot_initialize(inode);
> if ((ia_valid & ATTR_UID && !uid_eq(attr->ia_uid, inode->i_uid)) ||
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3356ab5..80877a48 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4455,6 +4455,10 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if (is_quota_modification(inode, attr))
> dquot_initialize(inode);
> if ((ia_valid & ATTR_UID && !uid_eq(attr->ia_uid, inode->i_uid)) ||
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 8e68bb6..c9371d2 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -560,6 +560,10 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
> if (err)
> return err;
>
> + err = setattr_killpriv(dentry, attr);
> + if (err)
> + return err;
> +
> if (attr->ia_valid & ATTR_SIZE) {
> err = f2fs_convert_inline_data(inode, attr->ia_size, NULL);
> if (err)
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index dbab798..f750848 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1693,9 +1693,10 @@ int fuse_flush_times(struct inode *inode, struct fuse_file *ff)
> * vmtruncate() doesn't allow for this case, so do the rlimit checking
> * and the actual truncation by hand.
> */
> -int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> +int fuse_do_setattr(struct dentry *entry, struct iattr *attr,
> struct file *file)
> {
> + struct inode *inode = entry->d_inode;
> struct fuse_conn *fc = get_fuse_conn(inode);
> struct fuse_inode *fi = get_fuse_inode(inode);
> struct fuse_req *req;
> @@ -1714,6 +1715,10 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> if (err)
> return err;
>
> + err = setattr_killpriv(entry, attr);
> + if (err)
> + return err;
> +
> if (attr->ia_valid & ATTR_OPEN) {
> if (fc->atomic_o_trunc)
> return 0;
> @@ -1809,15 +1814,13 @@ error:
>
> static int fuse_setattr(struct dentry *entry, struct iattr *attr)
> {
> - struct inode *inode = entry->d_inode;
> -
> - if (!fuse_allow_current_process(get_fuse_conn(inode)))
> + if (!fuse_allow_current_process(get_fuse_conn(entry->d_inode)))
> return -EACCES;
>
> if (attr->ia_valid & ATTR_FILE)
> - return fuse_do_setattr(inode, attr, attr->ia_file);
> + return fuse_do_setattr(entry, attr, attr->ia_file);
> else
> - return fuse_do_setattr(inode, attr, NULL);
> + return fuse_do_setattr(entry, attr, NULL);
> }
>
> static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index caa8d95..ffdc363 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2855,7 +2855,8 @@ static void fuse_do_truncate(struct file *file)
> attr.ia_file = file;
> attr.ia_valid |= ATTR_FILE;
>
> - fuse_do_setattr(inode, &attr, file);
> + /* XXX Is file->f_dentry->d_inode always the same as inode? */
> + fuse_do_setattr(file->f_dentry, &attr, file);
> }
>
> static inline loff_t fuse_round_up(loff_t off)
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index e8e47a6..163de1f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -894,7 +894,7 @@ bool fuse_write_update_size(struct inode *inode, loff_t pos);
> int fuse_flush_times(struct inode *inode, struct fuse_file *ff);
> int fuse_write_inode(struct inode *inode, struct writeback_control *wbc);
>
> -int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> +int fuse_do_setattr(struct dentry *entry, struct iattr *attr,
> struct file *file);
>
> #endif /* _FS_FUSE_I_H */
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index c4ed823..b39d81a 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -1786,6 +1786,9 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> goto out;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + goto out;
> if (attr->ia_valid & ATTR_SIZE)
> error = gfs2_setattr_size(inode, attr->ia_size);
> else if (attr->ia_valid & (ATTR_UID | ATTR_GID))
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index d0929bc..817f7a5 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -620,6 +620,10 @@ int hfs_inode_setattr(struct dentry *dentry, struct iattr * attr)
> return hsb->s_quiet ? 0 : error;
> }
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if (attr->ia_valid & ATTR_MODE) {
> /* Only the 'w' bits can ever change and only all together. */
> if (attr->ia_mode & S_IWUSR)
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index 0cf786f..12549bc 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -251,6 +251,10 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if ((attr->ia_valid & ATTR_SIZE) &&
> attr->ia_size != i_size_read(inode)) {
> inode_dio_wait(inode);
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index 601afd1..b260789 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -197,6 +197,10 @@ int jffs2_setattr(struct dentry *dentry, struct iattr *iattr)
> if (rc)
> return rc;
>
> + rc = setattr_killpriv(dentry, iattr);
> + if (rc)
> + return rc;
> +
> rc = jffs2_do_setattr(inode, iattr);
> if (!rc && (iattr->ia_valid & ATTR_MODE))
> rc = posix_acl_chmod(inode, inode->i_mode);
> diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> index 33aa0cc..4008313 100644
> --- a/fs/jfs/file.c
> +++ b/fs/jfs/file.c
> @@ -107,6 +107,10 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr)
> if (rc)
> return rc;
>
> + rc = setattr_killpriv(dentry, iattr);
> + if (rc)
> + return rc;
> +
> if (is_quota_modification(inode, iattr))
> dquot_initialize(inode);
> if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 9852176..6a70fc5 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -135,6 +135,23 @@ int kernfs_iop_setattr(struct dentry *dentry, struct iattr *iattr)
> if (error)
> goto out;
>
> + /*
> + * If we need to remove privileges, drop the mutex to do that
> + * first and then re-validate the remaining changes.
> + */
> + if (iattr->ia_valid & ATTR_KILL_PRIV) {
> + mutex_unlock(&kernfs_mutex);
> +
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> +
> + mutex_lock(&kernfs_mutex);
> + error = inode_change_ok(inode, iattr);
> + if (error)
> + goto out;
> + }
> +
> error = __kernfs_setattr(kn, iattr);
> if (error)
> goto out;
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 171d284..9a00049 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -375,6 +375,9 @@ int simple_setattr(struct dentry *dentry, struct iattr *iattr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> if (iattr->ia_valid & ATTR_SIZE)
> truncate_setsize(inode, iattr->ia_size);
> setattr_copy(inode, iattr);
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 00689a8..94dd6ac 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -496,7 +496,7 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
> {
> struct inode *inode = dentry->d_inode;
> struct nfs_fattr *fattr;
> - int error = -ENOMEM;
> + int error;
>
> nfs_inc_stats(inode, NFSIOS_VFSSETATTR);
>
> @@ -524,9 +524,16 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
> nfs_wb_all(inode);
> }
>
> + /* XXX Can we assume the server's permission checks are sufficient? */
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + goto out;
> +
> fattr = nfs_alloc_fattr();
> - if (fattr == NULL)
> + if (fattr == NULL) {
> + error = -ENOMEM;
> goto out;
> + }
> /*
> * Return any delegations if we're going to change ACLs
> */
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 324dc93..ed93d74 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1139,7 +1139,7 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
> attr->ia_valid &= ~ATTR_SIZE;
>
> #define OCFS2_VALID_ATTRS (ATTR_ATIME | ATTR_MTIME | ATTR_CTIME | ATTR_SIZE \
> - | ATTR_GID | ATTR_UID | ATTR_MODE)
> + | ATTR_GID | ATTR_UID | ATTR_MODE | ATTR_KILL_PRIV)
> if (!(attr->ia_valid & OCFS2_VALID_ATTRS))
> return 0;
>
> @@ -1147,6 +1147,10 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
> if (status)
> return status;
>
> + status = setattr_killpriv(dentry, attr);
> + if (status)
> + return status;
> +
> if (is_quota_modification(inode, attr))
> dquot_initialize(inode);
> size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE;
> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index a7eec98..a458c12 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -3316,6 +3316,10 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> /* must be turned off for recursive notify_change calls */
> ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
>
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index b5b593c..73d2e87 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1269,6 +1269,10 @@ int ubifs_setattr(struct dentry *dentry, struct iattr *attr)
> if (err)
> return err;
>
> + err = setattr_killpriv(dentry, attr);
> + if (err)
> + return err;
> +
> if ((attr->ia_valid & ATTR_SIZE) && attr->ia_size < inode->i_size)
> /* Truncation to a smaller size */
> err = do_truncation(c, inode, attr);
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index a65fa5d..22b7482 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -244,7 +244,8 @@ xfs_set_mode(struct inode *inode, umode_t mode)
> iattr.ia_mode = mode;
> iattr.ia_ctime = current_fs_time(inode->i_sb);
>
> - error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
> + error = xfs_setattr_nonsize(NULL, XFS_I(inode), &iattr,
> + XFS_ATTR_NOACL);
> }
>
> return error;
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index eb596b4..c9b9019 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -873,7 +873,7 @@ xfs_file_fallocate(
>
> iattr.ia_valid = ATTR_SIZE;
> iattr.ia_size = new_size;
> - error = xfs_setattr_size(ip, &iattr);
> + error = xfs_setattr_size(NULL, ip, &iattr);
> }
>
> out_unlock:
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 24c926b6..3e0dc56 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -714,7 +714,7 @@ xfs_ioc_space(
> iattr.ia_valid = ATTR_SIZE;
> iattr.ia_size = bf->l_start;
>
> - error = xfs_setattr_size(ip, &iattr);
> + error = xfs_setattr_size(NULL, ip, &iattr);
> if (!error)
> clrprealloc = true;
> break;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ec6dcdc..669150b 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -527,6 +527,7 @@ xfs_setattr_time(
>
> int
> xfs_setattr_nonsize(
> + struct dentry *dentry,
> struct xfs_inode *ip,
> struct iattr *iattr,
> int flags)
> @@ -554,6 +555,10 @@ xfs_setattr_nonsize(
> error = inode_change_ok(inode, iattr);
> if (error)
> return error;
> +
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> }
>
> ASSERT((mask & ATTR_SIZE) == 0);
> @@ -734,6 +739,7 @@ out_dqrele:
> */
> int
> xfs_setattr_size(
> + struct dentry *dentry,
> struct xfs_inode *ip,
> struct iattr *iattr)
> {
> @@ -776,9 +782,13 @@ xfs_setattr_size(
> * Use the regular setattr path to update the timestamps.
> */
> iattr->ia_valid &= ~ATTR_SIZE;
> - return xfs_setattr_nonsize(ip, iattr, 0);
> + return xfs_setattr_nonsize(dentry, ip, iattr, 0);
> }
>
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> +
> /*
> * Make sure that the dquots are attached to the inode.
> */
> @@ -974,10 +984,10 @@ xfs_vn_setattr(
>
> if (iattr->ia_valid & ATTR_SIZE) {
> xfs_ilock(ip, XFS_IOLOCK_EXCL);
> - error = xfs_setattr_size(ip, iattr);
> + error = xfs_setattr_size(dentry, ip, iattr);
> xfs_iunlock(ip, XFS_IOLOCK_EXCL);
> } else {
> - error = xfs_setattr_nonsize(ip, iattr, 0);
> + error = xfs_setattr_nonsize(dentry, ip, iattr, 0);
> }
>
> return error;
> diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
> index 1c34e43..6994d3e 100644
> --- a/fs/xfs/xfs_iops.h
> +++ b/fs/xfs/xfs_iops.h
> @@ -32,8 +32,14 @@ extern void xfs_setup_inode(struct xfs_inode *);
> */
> #define XFS_ATTR_NOACL 0x01 /* Don't call posix_acl_chmod */
>
> -extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
> +/*
> + * XXX Several callers have to pass dentry = NULL and this should
> + * work but it's really ugly.
> + */
> +extern int xfs_setattr_nonsize(struct dentry *dentry,
> + struct xfs_inode *ip, struct iattr *vap,
> int flags);
> -extern int xfs_setattr_size(struct xfs_inode *ip, struct iattr *vap);
> +extern int xfs_setattr_size(struct dentry *dentry,
> + struct xfs_inode *ip, struct iattr *vap);
>
> #endif /* __XFS_IOPS_H__ */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9ab779e..7cad5d1 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2663,6 +2663,7 @@ extern int buffer_migrate_page(struct address_space *,
> extern int inode_change_ok(const struct inode *, struct iattr *);
> extern int inode_newsize_ok(const struct inode *, loff_t offset);
> extern void setattr_copy(struct inode *inode, const struct iattr *attr);
> +extern int setattr_killpriv(struct dentry *dentry, struct iattr *attr);
>
> extern int file_update_time(struct file *file);
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 185836b..d1d4b9b 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -557,6 +557,10 @@ static int shmem_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
> loff_t oldsize = inode->i_size;
> loff_t newsize = attr->ia_size;
>
>
> --
> Ben Hutchings
> The first rule of tautology club is the first rule of tautology club.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
2015-01-17 23:26 [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks Ben Hutchings
2015-01-20 23:17 ` James Morris
2015-02-16 19:50 ` Josh Boyer
@ 2015-04-08 21:43 ` Mateusz Guzik
2015-04-13 1:39 ` James Morris
2 siblings, 1 reply; 9+ messages in thread
From: Mateusz Guzik @ 2015-04-08 21:43 UTC (permalink / raw)
To: Ben Hutchings
Cc: Alexander Viro, linux-fsdevel, linux-security-module, LKML,
770492, Ben Harris, oss-security
On Sat, Jan 17, 2015 at 11:26:46PM +0000, Ben Hutchings wrote:
> chown() and write() should clear all privilege attributes on
> a file - setuid, setgid, setcap and any other extended
> privilege attributes.
>
> However, any attributes beyond setuid and setgid are managed by the
> LSM and not directly by the filesystem, so they cannot be set along
> with the other attributes.
>
> Currently we call security_inode_killpriv() in notify_change(),
> but in case of a chown() this is too early - we have not called
> inode_change_ok() or made any filesystem-specific permission/sanity
> checks.
>
> Add a new function setattr_killpriv() which calls
> security_inode_killpriv() if necessary, and change the setattr()
> implementation to call this in each filesystem that supports xattrs.
> This assumes that extended privilege attributes are always stored in
> xattrs.
>
> Compile-tested only.
>
> XXX This is a silent change to the VFS API, but we should probably
> change something so OOT filesystems fail to compile if they aren't
> updated to call setattr_killpriv().
>
This is still a problem. Any feedback about the patch?
> Reported-by: Ben Harris <bjh21@cam.ac.uk>
> References: https://bugs.debian.org/770492
> ---
> drivers/staging/lustre/lustre/llite/llite_lib.c | 4 ++++
> fs/9p/vfs_inode.c | 4 ++++
> fs/9p/vfs_inode_dotl.c | 4 ++++
> fs/attr.c | 32 +++++++++++++++++++++----
> fs/btrfs/inode.c | 4 ++++
> fs/ceph/inode.c | 4 ++++
> fs/cifs/inode.c | 11 ++++++++-
> fs/ext2/inode.c | 4 ++++
> fs/ext3/inode.c | 4 ++++
> fs/ext4/inode.c | 4 ++++
> fs/f2fs/file.c | 4 ++++
> fs/fuse/dir.c | 15 +++++++-----
> fs/fuse/file.c | 3 ++-
> fs/fuse/fuse_i.h | 2 +-
> fs/gfs2/inode.c | 3 +++
> fs/hfs/inode.c | 4 ++++
> fs/hfsplus/inode.c | 4 ++++
> fs/jffs2/fs.c | 4 ++++
> fs/jfs/file.c | 4 ++++
> fs/kernfs/inode.c | 17 +++++++++++++
> fs/libfs.c | 3 +++
> fs/nfs/inode.c | 11 +++++++--
> fs/ocfs2/file.c | 6 ++++-
> fs/reiserfs/inode.c | 4 ++++
> fs/ubifs/file.c | 4 ++++
> fs/xfs/xfs_acl.c | 3 ++-
> fs/xfs/xfs_file.c | 2 +-
> fs/xfs/xfs_ioctl.c | 2 +-
> fs/xfs/xfs_iops.c | 16 ++++++++++---
> fs/xfs/xfs_iops.h | 10 ++++++--
> include/linux/fs.h | 1 +
> mm/shmem.c | 4 ++++
> 32 files changed, 176 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
> index a8bcc51..2a714b2 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_lib.c
> +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
> @@ -1434,6 +1434,10 @@ int ll_setattr_raw(struct dentry *dentry, struct iattr *attr, bool hsm_import)
> spin_unlock(&lli->lli_lock);
> }
>
> + rc = setattr_killpriv(dentry, attr);
> + if (rc)
> + return rc;
> +
> /* We always do an MDS RPC, even if we're only changing the size;
> * only the MDS knows whether truncate() should fail with -ETXTBUSY */
>
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 296482f..735cbf84 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -1130,6 +1130,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
> if (S_ISREG(dentry->d_inode->i_mode))
> filemap_write_and_wait(dentry->d_inode->i_mapping);
>
> + retval = setattr_killpriv(dentry, iattr);
> + if (retval)
> + return retval;
> +
> retval = p9_client_wstat(fid, &wstat);
> if (retval < 0)
> return retval;
> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 02b64f4..f3ca76d 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -583,6 +583,10 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr)
> if (S_ISREG(inode->i_mode))
> filemap_write_and_wait(inode->i_mapping);
>
> + retval = setattr_killpriv(dentry, iattr);
> + if (retval)
> + return retval;
> +
> retval = p9_client_setattr(fid, &p9attr);
> if (retval < 0)
> return retval;
> diff --git a/fs/attr.c b/fs/attr.c
> index 6530ced..184f3bf 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -168,6 +168,28 @@ void setattr_copy(struct inode *inode, const struct iattr *attr)
> EXPORT_SYMBOL(setattr_copy);
>
> /**
> + * setattr_killpriv - remove extended privilege attributes from a file
> + * @dentry: Directory entry passed to the setattr operation
> + * @iattr: New attributes pased to the setattr operation
> + *
> + * All filesystems that can carry extended privilege attributes
> + * should call this from their setattr operation *after* validating
> + * the attribute changes.
> + *
> + * It does nothing if !(iattr->ia_valid & ATTR_KILL_PRIV), so
> + * it is not necessary to call it in that case.
> + */
> +int setattr_killpriv(struct dentry *dentry, struct iattr *iattr)
> +{
> + if (!(iattr->ia_valid & ATTR_KILL_PRIV))
> + return 0;
> +
> + iattr->ia_valid &= ~ATTR_KILL_PRIV;
> + return security_inode_killpriv(dentry);
> +}
> +EXPORT_SYMBOL(setattr_killpriv);
> +
> +/**
> * notify_change - modify attributes of a filesytem object
> * @dentry: object affected
> * @iattr: new attributes
> @@ -217,13 +239,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
> if (!(ia_valid & ATTR_MTIME_SET))
> attr->ia_mtime = now;
> if (ia_valid & ATTR_KILL_PRIV) {
> - attr->ia_valid &= ~ATTR_KILL_PRIV;
> - ia_valid &= ~ATTR_KILL_PRIV;
> error = security_inode_need_killpriv(dentry);
> - if (error > 0)
> - error = security_inode_killpriv(dentry);
> - if (error)
> + if (error < 0)
> return error;
> + if (error == 0) {
> + attr->ia_valid &= ~ATTR_KILL_PRIV;
> + ia_valid &= ~ATTR_KILL_PRIV;
> + }
> }
>
> /*
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d23362f..71e3fb8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4697,6 +4697,10 @@ static int btrfs_setattr(struct dentry *dentry, struct iattr *attr)
> if (err)
> return err;
>
> + err = setattr_killpriv(dentry, attr);
> + if (err)
> + return err;
> +
> if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
> err = btrfs_setsize(inode, attr);
> if (err)
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 7b61390..9ba5556 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1712,6 +1712,10 @@ int ceph_setattr(struct dentry *dentry, struct iattr *attr)
> if (err != 0)
> return err;
>
> + err = setattr_killpriv(dentry, attr);
> + if (err != 0)
> + return err;
> +
> req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_SETATTR,
> USE_AUTH_MDS);
> if (IS_ERR(req))
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 197cb50..0e971f9 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -2149,7 +2149,10 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
> */
> rc = filemap_write_and_wait(inode->i_mapping);
> mapping_set_error(inode->i_mapping, rc);
> - rc = 0;
> +
> + rc = setattr_killpriv(direntry, attrs);
> + if (rc)
> + goto out;
>
> if (attrs->ia_valid & ATTR_SIZE) {
> rc = cifs_set_file_size(inode, attrs, xid, full_path);
> @@ -2273,6 +2276,12 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
> return rc;
> }
>
> + rc = setattr_killpriv(direntry, attrs);
> + if (rc) {
> + free_xid(xid);
> + return rc;
> + }
> +
> full_path = build_path_from_dentry(direntry);
> if (full_path == NULL) {
> rc = -ENOMEM;
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 36d35c3..9e245af 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1551,6 +1551,10 @@ int ext2_setattr(struct dentry *dentry, struct iattr *iattr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> +
> if (is_quota_modification(inode, iattr))
> dquot_initialize(inode);
> if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index 2c6ccc4..ec4dffa 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -3248,6 +3248,10 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if (is_quota_modification(inode, attr))
> dquot_initialize(inode);
> if ((ia_valid & ATTR_UID && !uid_eq(attr->ia_uid, inode->i_uid)) ||
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3356ab5..80877a48 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4455,6 +4455,10 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if (is_quota_modification(inode, attr))
> dquot_initialize(inode);
> if ((ia_valid & ATTR_UID && !uid_eq(attr->ia_uid, inode->i_uid)) ||
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 8e68bb6..c9371d2 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -560,6 +560,10 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
> if (err)
> return err;
>
> + err = setattr_killpriv(dentry, attr);
> + if (err)
> + return err;
> +
> if (attr->ia_valid & ATTR_SIZE) {
> err = f2fs_convert_inline_data(inode, attr->ia_size, NULL);
> if (err)
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index dbab798..f750848 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1693,9 +1693,10 @@ int fuse_flush_times(struct inode *inode, struct fuse_file *ff)
> * vmtruncate() doesn't allow for this case, so do the rlimit checking
> * and the actual truncation by hand.
> */
> -int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> +int fuse_do_setattr(struct dentry *entry, struct iattr *attr,
> struct file *file)
> {
> + struct inode *inode = entry->d_inode;
> struct fuse_conn *fc = get_fuse_conn(inode);
> struct fuse_inode *fi = get_fuse_inode(inode);
> struct fuse_req *req;
> @@ -1714,6 +1715,10 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> if (err)
> return err;
>
> + err = setattr_killpriv(entry, attr);
> + if (err)
> + return err;
> +
> if (attr->ia_valid & ATTR_OPEN) {
> if (fc->atomic_o_trunc)
> return 0;
> @@ -1809,15 +1814,13 @@ error:
>
> static int fuse_setattr(struct dentry *entry, struct iattr *attr)
> {
> - struct inode *inode = entry->d_inode;
> -
> - if (!fuse_allow_current_process(get_fuse_conn(inode)))
> + if (!fuse_allow_current_process(get_fuse_conn(entry->d_inode)))
> return -EACCES;
>
> if (attr->ia_valid & ATTR_FILE)
> - return fuse_do_setattr(inode, attr, attr->ia_file);
> + return fuse_do_setattr(entry, attr, attr->ia_file);
> else
> - return fuse_do_setattr(inode, attr, NULL);
> + return fuse_do_setattr(entry, attr, NULL);
> }
>
> static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index caa8d95..ffdc363 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2855,7 +2855,8 @@ static void fuse_do_truncate(struct file *file)
> attr.ia_file = file;
> attr.ia_valid |= ATTR_FILE;
>
> - fuse_do_setattr(inode, &attr, file);
> + /* XXX Is file->f_dentry->d_inode always the same as inode? */
> + fuse_do_setattr(file->f_dentry, &attr, file);
> }
>
> static inline loff_t fuse_round_up(loff_t off)
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index e8e47a6..163de1f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -894,7 +894,7 @@ bool fuse_write_update_size(struct inode *inode, loff_t pos);
> int fuse_flush_times(struct inode *inode, struct fuse_file *ff);
> int fuse_write_inode(struct inode *inode, struct writeback_control *wbc);
>
> -int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> +int fuse_do_setattr(struct dentry *entry, struct iattr *attr,
> struct file *file);
>
> #endif /* _FS_FUSE_I_H */
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index c4ed823..b39d81a 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -1786,6 +1786,9 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> goto out;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + goto out;
> if (attr->ia_valid & ATTR_SIZE)
> error = gfs2_setattr_size(inode, attr->ia_size);
> else if (attr->ia_valid & (ATTR_UID | ATTR_GID))
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index d0929bc..817f7a5 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -620,6 +620,10 @@ int hfs_inode_setattr(struct dentry *dentry, struct iattr * attr)
> return hsb->s_quiet ? 0 : error;
> }
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if (attr->ia_valid & ATTR_MODE) {
> /* Only the 'w' bits can ever change and only all together. */
> if (attr->ia_mode & S_IWUSR)
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index 0cf786f..12549bc 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -251,6 +251,10 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if ((attr->ia_valid & ATTR_SIZE) &&
> attr->ia_size != i_size_read(inode)) {
> inode_dio_wait(inode);
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index 601afd1..b260789 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -197,6 +197,10 @@ int jffs2_setattr(struct dentry *dentry, struct iattr *iattr)
> if (rc)
> return rc;
>
> + rc = setattr_killpriv(dentry, iattr);
> + if (rc)
> + return rc;
> +
> rc = jffs2_do_setattr(inode, iattr);
> if (!rc && (iattr->ia_valid & ATTR_MODE))
> rc = posix_acl_chmod(inode, inode->i_mode);
> diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> index 33aa0cc..4008313 100644
> --- a/fs/jfs/file.c
> +++ b/fs/jfs/file.c
> @@ -107,6 +107,10 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr)
> if (rc)
> return rc;
>
> + rc = setattr_killpriv(dentry, iattr);
> + if (rc)
> + return rc;
> +
> if (is_quota_modification(inode, iattr))
> dquot_initialize(inode);
> if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 9852176..6a70fc5 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -135,6 +135,23 @@ int kernfs_iop_setattr(struct dentry *dentry, struct iattr *iattr)
> if (error)
> goto out;
>
> + /*
> + * If we need to remove privileges, drop the mutex to do that
> + * first and then re-validate the remaining changes.
> + */
> + if (iattr->ia_valid & ATTR_KILL_PRIV) {
> + mutex_unlock(&kernfs_mutex);
> +
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> +
> + mutex_lock(&kernfs_mutex);
> + error = inode_change_ok(inode, iattr);
> + if (error)
> + goto out;
> + }
> +
> error = __kernfs_setattr(kn, iattr);
> if (error)
> goto out;
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 171d284..9a00049 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -375,6 +375,9 @@ int simple_setattr(struct dentry *dentry, struct iattr *iattr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> if (iattr->ia_valid & ATTR_SIZE)
> truncate_setsize(inode, iattr->ia_size);
> setattr_copy(inode, iattr);
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 00689a8..94dd6ac 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -496,7 +496,7 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
> {
> struct inode *inode = dentry->d_inode;
> struct nfs_fattr *fattr;
> - int error = -ENOMEM;
> + int error;
>
> nfs_inc_stats(inode, NFSIOS_VFSSETATTR);
>
> @@ -524,9 +524,16 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
> nfs_wb_all(inode);
> }
>
> + /* XXX Can we assume the server's permission checks are sufficient? */
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + goto out;
> +
> fattr = nfs_alloc_fattr();
> - if (fattr == NULL)
> + if (fattr == NULL) {
> + error = -ENOMEM;
> goto out;
> + }
> /*
> * Return any delegations if we're going to change ACLs
> */
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 324dc93..ed93d74 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1139,7 +1139,7 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
> attr->ia_valid &= ~ATTR_SIZE;
>
> #define OCFS2_VALID_ATTRS (ATTR_ATIME | ATTR_MTIME | ATTR_CTIME | ATTR_SIZE \
> - | ATTR_GID | ATTR_UID | ATTR_MODE)
> + | ATTR_GID | ATTR_UID | ATTR_MODE | ATTR_KILL_PRIV)
> if (!(attr->ia_valid & OCFS2_VALID_ATTRS))
> return 0;
>
> @@ -1147,6 +1147,10 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
> if (status)
> return status;
>
> + status = setattr_killpriv(dentry, attr);
> + if (status)
> + return status;
> +
> if (is_quota_modification(inode, attr))
> dquot_initialize(inode);
> size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE;
> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index a7eec98..a458c12 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -3316,6 +3316,10 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> /* must be turned off for recursive notify_change calls */
> ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
>
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index b5b593c..73d2e87 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1269,6 +1269,10 @@ int ubifs_setattr(struct dentry *dentry, struct iattr *attr)
> if (err)
> return err;
>
> + err = setattr_killpriv(dentry, attr);
> + if (err)
> + return err;
> +
> if ((attr->ia_valid & ATTR_SIZE) && attr->ia_size < inode->i_size)
> /* Truncation to a smaller size */
> err = do_truncation(c, inode, attr);
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index a65fa5d..22b7482 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -244,7 +244,8 @@ xfs_set_mode(struct inode *inode, umode_t mode)
> iattr.ia_mode = mode;
> iattr.ia_ctime = current_fs_time(inode->i_sb);
>
> - error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
> + error = xfs_setattr_nonsize(NULL, XFS_I(inode), &iattr,
> + XFS_ATTR_NOACL);
> }
>
> return error;
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index eb596b4..c9b9019 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -873,7 +873,7 @@ xfs_file_fallocate(
>
> iattr.ia_valid = ATTR_SIZE;
> iattr.ia_size = new_size;
> - error = xfs_setattr_size(ip, &iattr);
> + error = xfs_setattr_size(NULL, ip, &iattr);
> }
>
> out_unlock:
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 24c926b6..3e0dc56 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -714,7 +714,7 @@ xfs_ioc_space(
> iattr.ia_valid = ATTR_SIZE;
> iattr.ia_size = bf->l_start;
>
> - error = xfs_setattr_size(ip, &iattr);
> + error = xfs_setattr_size(NULL, ip, &iattr);
> if (!error)
> clrprealloc = true;
> break;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ec6dcdc..669150b 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -527,6 +527,7 @@ xfs_setattr_time(
>
> int
> xfs_setattr_nonsize(
> + struct dentry *dentry,
> struct xfs_inode *ip,
> struct iattr *iattr,
> int flags)
> @@ -554,6 +555,10 @@ xfs_setattr_nonsize(
> error = inode_change_ok(inode, iattr);
> if (error)
> return error;
> +
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> }
>
> ASSERT((mask & ATTR_SIZE) == 0);
> @@ -734,6 +739,7 @@ out_dqrele:
> */
> int
> xfs_setattr_size(
> + struct dentry *dentry,
> struct xfs_inode *ip,
> struct iattr *iattr)
> {
> @@ -776,9 +782,13 @@ xfs_setattr_size(
> * Use the regular setattr path to update the timestamps.
> */
> iattr->ia_valid &= ~ATTR_SIZE;
> - return xfs_setattr_nonsize(ip, iattr, 0);
> + return xfs_setattr_nonsize(dentry, ip, iattr, 0);
> }
>
> + error = setattr_killpriv(dentry, iattr);
> + if (error)
> + return error;
> +
> /*
> * Make sure that the dquots are attached to the inode.
> */
> @@ -974,10 +984,10 @@ xfs_vn_setattr(
>
> if (iattr->ia_valid & ATTR_SIZE) {
> xfs_ilock(ip, XFS_IOLOCK_EXCL);
> - error = xfs_setattr_size(ip, iattr);
> + error = xfs_setattr_size(dentry, ip, iattr);
> xfs_iunlock(ip, XFS_IOLOCK_EXCL);
> } else {
> - error = xfs_setattr_nonsize(ip, iattr, 0);
> + error = xfs_setattr_nonsize(dentry, ip, iattr, 0);
> }
>
> return error;
> diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
> index 1c34e43..6994d3e 100644
> --- a/fs/xfs/xfs_iops.h
> +++ b/fs/xfs/xfs_iops.h
> @@ -32,8 +32,14 @@ extern void xfs_setup_inode(struct xfs_inode *);
> */
> #define XFS_ATTR_NOACL 0x01 /* Don't call posix_acl_chmod */
>
> -extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
> +/*
> + * XXX Several callers have to pass dentry = NULL and this should
> + * work but it's really ugly.
> + */
> +extern int xfs_setattr_nonsize(struct dentry *dentry,
> + struct xfs_inode *ip, struct iattr *vap,
> int flags);
> -extern int xfs_setattr_size(struct xfs_inode *ip, struct iattr *vap);
> +extern int xfs_setattr_size(struct dentry *dentry,
> + struct xfs_inode *ip, struct iattr *vap);
>
> #endif /* __XFS_IOPS_H__ */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9ab779e..7cad5d1 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2663,6 +2663,7 @@ extern int buffer_migrate_page(struct address_space *,
> extern int inode_change_ok(const struct inode *, struct iattr *);
> extern int inode_newsize_ok(const struct inode *, loff_t offset);
> extern void setattr_copy(struct inode *inode, const struct iattr *attr);
> +extern int setattr_killpriv(struct dentry *dentry, struct iattr *attr);
>
> extern int file_update_time(struct file *file);
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 185836b..d1d4b9b 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -557,6 +557,10 @@ static int shmem_setattr(struct dentry *dentry, struct iattr *attr)
> if (error)
> return error;
>
> + error = setattr_killpriv(dentry, attr);
> + if (error)
> + return error;
> +
> if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
> loff_t oldsize = inode->i_size;
> loff_t newsize = attr->ia_size;
>
>
> --
> Ben Hutchings
> The first rule of tautology club is the first rule of tautology club.
--
Mateusz Guzik
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
2015-04-08 21:43 ` Mateusz Guzik
@ 2015-04-13 1:39 ` James Morris
2015-06-03 17:57 ` Mateusz Guzik
0 siblings, 1 reply; 9+ messages in thread
From: James Morris @ 2015-04-13 1:39 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Ben Hutchings, Alexander Viro, linux-fsdevel,
linux-security-module, LKML, 770492, Ben Harris, oss-security
On Wed, 8 Apr 2015, Mateusz Guzik wrote:
> This is still a problem. Any feedback about the patch?
>
I'd like to see feedback from vfs folk (Al).
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
2015-04-13 1:39 ` James Morris
@ 2015-06-03 17:57 ` Mateusz Guzik
0 siblings, 0 replies; 9+ messages in thread
From: Mateusz Guzik @ 2015-06-03 17:57 UTC (permalink / raw)
To: James Morris
Cc: Ben Hutchings, Alexander Viro, linux-fsdevel,
linux-security-module, LKML, 770492, Ben Harris
On Mon, Apr 13, 2015 at 11:39:01AM +1000, James Morris wrote:
> On Wed, 8 Apr 2015, Mateusz Guzik wrote:
>
> > This is still a problem. Any feedback about the patch?
> >
>
> I'd like to see feedback from vfs folk (Al).
>
Ping? Are there any concerns with the patch?
--
Mateusz Guzik
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-06-03 17:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-17 23:26 [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks Ben Hutchings
2015-01-20 23:17 ` James Morris
2015-01-20 23:32 ` Casey Schaufler
2015-01-21 14:03 ` Stephen Smalley
2015-01-21 16:15 ` Casey Schaufler
2015-02-16 19:50 ` Josh Boyer
2015-04-08 21:43 ` Mateusz Guzik
2015-04-13 1:39 ` James Morris
2015-06-03 17:57 ` Mateusz Guzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).