* [PATCH 1/5] fs: Fix S_NOSEC handling
2015-05-21 14:05 [PATCH 0/5 v4] fs: Fixes for removing xid bits and security labels Jan Kara
@ 2015-05-21 14:05 ` Jan Kara
2015-05-21 14:05 ` [PATCH 2/5] fs: Rename file_remove_suid() to file_remove_privs() Jan Kara
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jan Kara @ 2015-05-21 14:05 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, linux-fsdevel, dchinner, Serge Hallyn,
linux-security-module, Jan Kara, stable
file_remove_suid() could mistakenly set S_NOSEC inode bit when root was
modifying the file. As a result following writes to the file by ordinary
user would avoid clearing suid or sgid bits.
Fix the bug by checking actual mode bits before setting S_NOSEC.
CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/inode.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index ea37cd17b53f..6e342cadef81 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1693,8 +1693,8 @@ int file_remove_suid(struct file *file)
error = security_inode_killpriv(dentry);
if (!error && killsuid)
error = __remove_suid(dentry, killsuid);
- if (!error && (inode->i_sb->s_flags & MS_NOSEC))
- inode->i_flags |= S_NOSEC;
+ if (!error)
+ inode_has_no_xattr(inode);
return error;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/5] fs: Rename file_remove_suid() to file_remove_privs()
2015-05-21 14:05 [PATCH 0/5 v4] fs: Fixes for removing xid bits and security labels Jan Kara
2015-05-21 14:05 ` [PATCH 1/5] fs: Fix S_NOSEC handling Jan Kara
@ 2015-05-21 14:05 ` Jan Kara
2015-05-21 14:05 ` [PATCH 3/5] fs: Provide function telling whether file_remove_privs() will do anything Jan Kara
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jan Kara @ 2015-05-21 14:05 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, linux-fsdevel, dchinner, Serge Hallyn,
linux-security-module, Jan Kara
file_remove_suid() is a misnomer since it removes also file capabilities
stored in xattrs and sets S_NOSEC flag. Also should_remove_suid() tells
something else than whether file_remove_suid() call is necessary which
leads to bugs.
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/btrfs/file.c | 2 +-
fs/ceph/file.c | 2 +-
fs/fuse/file.c | 2 +-
fs/inode.c | 13 ++++++++-----
fs/ntfs/file.c | 2 +-
fs/xfs/xfs_file.c | 2 +-
include/linux/fs.h | 2 +-
mm/filemap.c | 2 +-
8 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index b072e17479aa..86f97282779a 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1748,7 +1748,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
}
current->backing_dev_info = inode_to_bdi(inode);
- err = file_remove_suid(file);
+ err = file_remove_privs(file);
if (err) {
mutex_unlock(&inode->i_mutex);
goto out;
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 3b6b522b4b31..e55fe32c6224 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -959,7 +959,7 @@ static ssize_t ceph_write_iter(struct kiocb *iocb, struct iov_iter *from)
pos = iocb->ki_pos;
count = iov_iter_count(from);
- err = file_remove_suid(file);
+ err = file_remove_privs(file);
if (err)
goto out;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 5ef05b5c4cff..1344647965dc 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1169,7 +1169,7 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
if (err <= 0)
goto out;
- err = file_remove_suid(file);
+ err = file_remove_privs(file);
if (err)
goto out;
diff --git a/fs/inode.c b/fs/inode.c
index 6e342cadef81..48313a1444fb 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1672,7 +1672,11 @@ static int __remove_suid(struct dentry *dentry, int kill)
return notify_change(dentry, &newattrs, NULL);
}
-int file_remove_suid(struct file *file)
+/*
+ * Remove special file priviledges (suid, capabilities) when file is written
+ * to or truncated.
+ */
+int file_remove_privs(struct file *file)
{
struct dentry *dentry = file->f_path.dentry;
struct inode *inode = d_inode(dentry);
@@ -1698,7 +1702,7 @@ int file_remove_suid(struct file *file)
return error;
}
-EXPORT_SYMBOL(file_remove_suid);
+EXPORT_SYMBOL(file_remove_privs);
/**
* file_update_time - update mtime and ctime time
@@ -1953,9 +1957,8 @@ EXPORT_SYMBOL(inode_dio_wait);
* inode is being instantiated). The reason for the cmpxchg() loop
* --- which wouldn't be necessary if all code paths which modify
* i_flags actually followed this rule, is that there is at least one
- * code path which doesn't today --- for example,
- * __generic_file_aio_write() calls file_remove_suid() without holding
- * i_mutex --- so we use cmpxchg() out of an abundance of caution.
+ * code path which doesn't today so we use cmpxchg() out of an abundance
+ * of caution.
*
* In the long run, i_mutex is overkill, and we should probably look
* at using the i_lock spinlock to protect i_flags, and then make sure
diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
index 7bb487e663b4..182bb93aa79c 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -382,7 +382,7 @@ static ssize_t ntfs_prepare_file_for_write(struct kiocb *iocb,
base_ni = ni;
if (NInoAttr(ni))
base_ni = ni->ext.base_ntfs_ino;
- err = file_remove_suid(file);
+ err = file_remove_privs(file);
if (unlikely(err))
goto out;
/*
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 8121e75352ee..f3e4fbb59985 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -623,7 +623,7 @@ restart:
* setgid bits if the process is not being run by root. This keeps
* people from modifying setuid and setgid binaries.
*/
- return file_remove_suid(file);
+ return file_remove_privs(file);
}
/*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 35ec87e490b1..082f66274e63 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2552,7 +2552,7 @@ extern struct inode *new_inode_pseudo(struct super_block *sb);
extern struct inode *new_inode(struct super_block *sb);
extern void free_inode_nonrcu(struct inode *inode);
extern int should_remove_suid(struct dentry *);
-extern int file_remove_suid(struct file *);
+extern int file_remove_privs(struct file *);
extern void __insert_inode_hash(struct inode *, unsigned long hashval);
static inline void insert_inode_hash(struct inode *inode)
diff --git a/mm/filemap.c b/mm/filemap.c
index 6bf5e42d560a..f851e36802d5 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2536,7 +2536,7 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
/* We can write back this queue in page reclaim */
current->backing_dev_info = inode_to_bdi(inode);
- err = file_remove_suid(file);
+ err = file_remove_privs(file);
if (err)
goto out;
--
2.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/5] fs: Provide function telling whether file_remove_privs() will do anything
2015-05-21 14:05 [PATCH 0/5 v4] fs: Fixes for removing xid bits and security labels Jan Kara
2015-05-21 14:05 ` [PATCH 1/5] fs: Fix S_NOSEC handling Jan Kara
2015-05-21 14:05 ` [PATCH 2/5] fs: Rename file_remove_suid() to file_remove_privs() Jan Kara
@ 2015-05-21 14:05 ` Jan Kara
2015-05-21 14:05 ` [PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate Jan Kara
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jan Kara @ 2015-05-21 14:05 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, linux-fsdevel, dchinner, Serge Hallyn,
linux-security-module, Jan Kara
Provide function telling whether file_remove_privs() will do anything.
Currently we only have should_remove_suid() and that does something
slightly different.
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/inode.c | 44 ++++++++++++++++++++++++++++++++------------
include/linux/fs.h | 1 +
2 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index 48313a1444fb..620cb7686242 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1660,7 +1660,32 @@ int should_remove_suid(struct dentry *dentry)
}
EXPORT_SYMBOL(should_remove_suid);
-static int __remove_suid(struct dentry *dentry, int kill)
+/*
+ * Return mask of changes for notify_change() that need to be done as a
+ * response to write or truncate. Return 0 if nothing has to be changed.
+ * Negative value on error (change should be denied).
+ */
+int file_needs_remove_privs(struct file *file)
+{
+ struct dentry *dentry = file->f_path.dentry;
+ struct inode *inode = d_inode(dentry);
+ int mask = 0;
+ int ret;
+
+ if (IS_NOSEC(inode))
+ return 0;
+
+ mask = should_remove_suid(dentry);
+ ret = security_inode_need_killpriv(dentry);
+ if (ret < 0)
+ return ret;
+ if (ret)
+ mask |= ATTR_KILL_PRIV;
+ return mask;
+}
+EXPORT_SYMBOL(file_needs_remove_privs);
+
+static int __remove_privs(struct dentry *dentry, int kill)
{
struct iattr newattrs;
@@ -1680,23 +1705,18 @@ int file_remove_privs(struct file *file)
{
struct dentry *dentry = file->f_path.dentry;
struct inode *inode = d_inode(dentry);
- int killsuid;
- int killpriv;
+ int kill;
int error = 0;
/* Fast path for nothing security related */
if (IS_NOSEC(inode))
return 0;
- killsuid = should_remove_suid(dentry);
- killpriv = security_inode_need_killpriv(dentry);
-
- if (killpriv < 0)
- return killpriv;
- if (killpriv)
- error = security_inode_killpriv(dentry);
- if (!error && killsuid)
- error = __remove_suid(dentry, killsuid);
+ kill = file_needs_remove_privs(file);
+ if (kill < 0)
+ return kill;
+ if (kill)
+ error = __remove_privs(dentry, kill);
if (!error)
inode_has_no_xattr(inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 082f66274e63..a0b21f1ae7cb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2553,6 +2553,7 @@ extern struct inode *new_inode(struct super_block *sb);
extern void free_inode_nonrcu(struct inode *inode);
extern int should_remove_suid(struct dentry *);
extern int file_remove_privs(struct file *);
+extern int file_needs_remove_privs(struct file *file);
extern void __insert_inode_hash(struct inode *, unsigned long hashval);
static inline void insert_inode_hash(struct inode *inode)
--
2.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate
2015-05-21 14:05 [PATCH 0/5 v4] fs: Fixes for removing xid bits and security labels Jan Kara
` (2 preceding siblings ...)
2015-05-21 14:05 ` [PATCH 3/5] fs: Provide function telling whether file_remove_privs() will do anything Jan Kara
@ 2015-05-21 14:05 ` Jan Kara
2015-05-21 14:05 ` [PATCH 5/5] xfs: Correctly lock inode when removing suid and file capabilities Jan Kara
2015-05-21 14:05 ` [PATCH 5/5] xfs: Correctly lock inode when removing suid and security marks Jan Kara
5 siblings, 0 replies; 10+ messages in thread
From: Jan Kara @ 2015-05-21 14:05 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, linux-fsdevel, dchinner, Serge Hallyn,
linux-security-module, Jan Kara
Comment in include/linux/security.h says that ->inode_killpriv() should
be called when setuid bit is being removed and that similar security
labels (in fact this applies only to file capabilities) should be
removed at this time as well. However we don't call ->inode_killpriv()
when we remove suid bit on truncate.
We fix the problem by calling ->inode_need_killpriv() and subsequently
->inode_killpriv() on truncate the same way as we do it on file write.
After this patch there's only one user of should_remove_suid() - ocfs2 -
and indeed it's buggy because it doesn't call ->inode_killpriv() on
write. However fixing it is difficult because of special locking
constraints.
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/inode.c | 5 ++---
fs/open.c | 6 ++++--
include/linux/fs.h | 6 +++++-
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index 620cb7686242..36982d74e967 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1665,9 +1665,8 @@ EXPORT_SYMBOL(should_remove_suid);
* response to write or truncate. Return 0 if nothing has to be changed.
* Negative value on error (change should be denied).
*/
-int file_needs_remove_privs(struct file *file)
+int dentry_needs_remove_privs(struct dentry *dentry)
{
- struct dentry *dentry = file->f_path.dentry;
struct inode *inode = d_inode(dentry);
int mask = 0;
int ret;
@@ -1683,7 +1682,7 @@ int file_needs_remove_privs(struct file *file)
mask |= ATTR_KILL_PRIV;
return mask;
}
-EXPORT_SYMBOL(file_needs_remove_privs);
+EXPORT_SYMBOL(dentry_needs_remove_privs);
static int __remove_privs(struct dentry *dentry, int kill)
{
diff --git a/fs/open.c b/fs/open.c
index 98e5a52dc68c..01e4aea78dcf 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -51,8 +51,10 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
newattrs.ia_valid |= ATTR_FILE;
}
- /* Remove suid/sgid on truncate too */
- ret = should_remove_suid(dentry);
+ /* Remove suid, sgid, and file capabilities on truncate too */
+ ret = dentry_needs_remove_privs(dentry);
+ if (ret < 0)
+ return ret;
if (ret)
newattrs.ia_valid |= ret | ATTR_FORCE;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a0b21f1ae7cb..cbbadaae03f7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2553,7 +2553,11 @@ extern struct inode *new_inode(struct super_block *sb);
extern void free_inode_nonrcu(struct inode *inode);
extern int should_remove_suid(struct dentry *);
extern int file_remove_privs(struct file *);
-extern int file_needs_remove_privs(struct file *file);
+extern int dentry_needs_remove_privs(struct dentry *dentry);
+static inline int file_needs_remove_privs(struct file *file)
+{
+ return dentry_needs_remove_privs(file->f_path.dentry);
+}
extern void __insert_inode_hash(struct inode *, unsigned long hashval);
static inline void insert_inode_hash(struct inode *inode)
--
2.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/5] xfs: Correctly lock inode when removing suid and file capabilities
2015-05-21 14:05 [PATCH 0/5 v4] fs: Fixes for removing xid bits and security labels Jan Kara
` (3 preceding siblings ...)
2015-05-21 14:05 ` [PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate Jan Kara
@ 2015-05-21 14:05 ` Jan Kara
2015-05-21 14:05 ` [PATCH 5/5] xfs: Correctly lock inode when removing suid and security marks Jan Kara
5 siblings, 0 replies; 10+ messages in thread
From: Jan Kara @ 2015-05-21 14:05 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, linux-fsdevel, dchinner, Serge Hallyn,
linux-security-module, Jan Kara
Currently XFS calls file_remove_privs() without holding i_mutex. This is
wrong because that function can end up messing with file permissions and
file capabilities stored in xattrs for which we need i_mutex held.
Fix the problem by grabbing iolock exclusively when we will need to
change anything in permissions / xattrs.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/xfs/xfs_file.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index f3e4fbb59985..71c2c712e609 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -563,6 +563,13 @@ restart:
if (error)
return error;
+ /* For changing security info in file_remove_privs() we need i_mutex */
+ if (*iolock == XFS_IOLOCK_SHARED && !IS_NOSEC(inode)) {
+ xfs_rw_iunlock(ip, *iolock);
+ *iolock = XFS_IOLOCK_EXCL;
+ xfs_rw_ilock(ip, *iolock);
+ goto restart;
+ }
/*
* If the offset is beyond the size of the file, we need to zero any
* blocks that fall between the existing EOF and the start of this
@@ -623,7 +630,9 @@ restart:
* setgid bits if the process is not being run by root. This keeps
* people from modifying setuid and setgid binaries.
*/
- return file_remove_privs(file);
+ if (!IS_NOSEC(inode))
+ return file_remove_privs(file);
+ return 0;
}
/*
--
2.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/5] xfs: Correctly lock inode when removing suid and security marks
2015-05-21 14:05 [PATCH 0/5 v4] fs: Fixes for removing xid bits and security labels Jan Kara
` (4 preceding siblings ...)
2015-05-21 14:05 ` [PATCH 5/5] xfs: Correctly lock inode when removing suid and file capabilities Jan Kara
@ 2015-05-21 14:05 ` Jan Kara
5 siblings, 0 replies; 10+ messages in thread
From: Jan Kara @ 2015-05-21 14:05 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, linux-fsdevel, dchinner, Serge Hallyn,
linux-security-module, Jan Kara
Currently XFS calls file_remove_privs() without holding i_mutex. This is
wrong because that function can end up messing with file permissions and
security xattrs for which we need i_mutex held.
Fix the problem by grabbing iolock exclusively when we will need to
change anything in permissions / xattrs.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/xfs/xfs_file.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index f3e4fbb59985..71c2c712e609 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -563,6 +563,13 @@ restart:
if (error)
return error;
+ /* For changing security info in file_remove_privs() we need i_mutex */
+ if (*iolock == XFS_IOLOCK_SHARED && !IS_NOSEC(inode)) {
+ xfs_rw_iunlock(ip, *iolock);
+ *iolock = XFS_IOLOCK_EXCL;
+ xfs_rw_ilock(ip, *iolock);
+ goto restart;
+ }
/*
* If the offset is beyond the size of the file, we need to zero any
* blocks that fall between the existing EOF and the start of this
@@ -623,7 +630,9 @@ restart:
* setgid bits if the process is not being run by root. This keeps
* people from modifying setuid and setgid binaries.
*/
- return file_remove_privs(file);
+ if (!IS_NOSEC(inode))
+ return file_remove_privs(file);
+ return 0;
}
/*
--
2.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread