[PATCH 0/5 v2 RESEND] fs: Fixes for removing xid bits and security labels

[PATCH 0/5 v2 RESEND] fs: Fixes for removing xid bits and security labels

[Jan Kara]

  Hello,

  warning in XFS made me look into detail into how clearing of suid / sgid
bits and security labels is done. And I've spotted a few issues:
1) MS_NOSEC handling is broken - we set it after each file_remove_suid() call.
   However we needn't have removed suid bit simply because we have
   CAP_SYS_FSID and further writes to the file from processes without this
   capability still need to clear the suid bit.
2) file_remove_suid() is a misnomer since it also handles removing of
   security labels. It is even more confusing because should_remove_suid()
   doesn't return whether file_remove_suid() is needed or not.
3) On truncate we do clear suid bits but not security labels. According to
   documentation in include/linux/security.h that's a bug but please correct
   me if I'm wrong.
4) ocfs2 doesn't clear security labels - hard to fix, I left it alone for now.
5) XFS didn't provide proper exclusion for clearing mode bits.

  This series aims at fixing above issues.

  Since v1 I have removed bogus patch changing inode_set_flags(), I have
updated changelog of patch 4/5 to better explain why ->inode_killpriv should
be called and I have included a fix for MS_NOSEC handling in this series.
Al, can you please merge the patches? Thanks!

								Honza

[PATCH 1/5] fs: Fix S_NOSEC handling

[Jan Kara]

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index f00b16f45507..be326ae7f880 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1693,7 +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))
+	if (!error && (inode->i_sb->s_flags & MS_NOSEC) &&
+	    !is_sxid(inode->i_mode))
 		inode->i_flags |= S_NOSEC;
 
 	return error;
-- 
2.1.4

[PATCH 2/5] fs: Rename file_remove_suid() to file_remove_privs()

[Jan Kara]

file_remove_suid() is a misnomer since it removes also security related
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 b78bbbac900d..c693dc2136e1 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1760,7 +1760,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 
 	iov_iter_truncate(from, count);
 
-	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 d533075a823d..493f293d433e 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -962,7 +962,7 @@ static ssize_t ceph_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		goto out;
 	iov_iter_truncate(from, count);
 
-	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 c01ec3bdcfd8..3e2ede73a3e3 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)
 		goto out;
 
 	iov_iter_truncate(from, count);
-	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 be326ae7f880..bc5fe06b8784 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, security tags) when file is written
+ * to or truncated.
+ */
+int file_remove_privs(struct file *file)
 {
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
@@ -1699,7 +1703,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
@@ -1968,9 +1972,8 @@ EXPORT_SYMBOL(inode_dio_done);
  * 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 1da9b2d184dc..008a476558d3 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -2099,7 +2099,7 @@ static ssize_t ntfs_file_aio_write_nolock(struct kiocb *iocb,
 		goto out;
 	if (!count)
 		goto out;
-	err = file_remove_suid(file);
+	err = file_remove_privs(file);
 	if (err)
 		goto out;
 	err = file_update_time(file);
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index a2e1cb8a568b..fa81a43702ee 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -601,7 +601,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 b4d71b5e1ff2..676e431247bd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2517,7 +2517,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 ad7242043bdb..ac021d604b1e 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2575,7 +2575,7 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 	iov_iter_truncate(from, count);
 
-	err = file_remove_suid(file);
+	err = file_remove_privs(file);
 	if (err)
 		goto out;
 
-- 
2.1.4

[PATCH 3/5] fs: Provide function telling whether file_remove_privs() will do anything

[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 bc5fe06b8784..3407bedc1c7b 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 = dentry->d_inode;
+	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 = dentry->d_inode;
-	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->i_sb->s_flags & MS_NOSEC) &&
 	    !is_sxid(inode->i_mode))
 		inode->i_flags |= S_NOSEC;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 676e431247bd..78ff19f0c323 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2518,6 +2518,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

[PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate

[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 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 clear security attributes 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 3407bedc1c7b..7f4eabd72bd6 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 = dentry->d_inode;
 	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 33f9cbf2610b..14e2aeea4cee 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 security markings 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 78ff19f0c323..ddaceede20ad 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2518,7 +2518,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

[PATCH 5/5] xfs: Correctly lock inode when removing suid and security marks

[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.

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 fa81a43702ee..3d601e92bd44 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
@@ -601,7 +608,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

Re: [PATCH 5/5] xfs: Correctly lock inode when removing suid and security marks

[Dave Chinner]

On Tue, Mar 03, 2015 at 11:38:39AM +0100, Jan Kara wrote:
> 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.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/5] xfs: Correctly lock inode when removing suid and security marks

[Eric Sandeen]

On 3/3/15 4:38 AM, Jan Kara wrote:
> 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.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

This seems like it stands alone, modulo the file_remove_privs function
renaming... should it just be pulled into XFS by Dave?  I guess that would
require the renaming patch to be rebased...

Thanks,
-Eric


> ---
>  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 fa81a43702ee..3d601e92bd44 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
> @@ -601,7 +608,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;
>  }
>  
>  /*
> 

Re: [PATCH 5/5] xfs: Correctly lock inode when removing suid and security marks

[Dave Chinner]

On Tue, Apr 14, 2015 at 11:54:55AM -0500, Eric Sandeen wrote:
> On 3/3/15 4:38 AM, Jan Kara wrote:
> > 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.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> This seems like it stands alone, modulo the file_remove_privs function
> renaming... should it just be pulled into XFS by Dave?  I guess that would
> require the renaming patch to be rebased...

The file_remove_privs() is introduced earlier in the series. With
all the changes pending to xfs_file_aio_write_checks() from the
upcoming VFS merge, the DIo write fixes I'll be pushing, this, and
other problems Al Viro has noticed, I'm probably going to have to
refactor this code for 4.1-rc2 to fix all the bugs we've introduced
during integration of all the changes...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/5 v2 RESEND] fs: Fixes for removing xid bits and security labels

[Dave Chinner]

On Tue, Mar 03, 2015 at 11:38:34AM +0100, Jan Kara wrote:
>   Hello,
> 
>   warning in XFS made me look into detail into how clearing of suid / sgid
> bits and security labels is done. And I've spotted a few issues:
> 1) MS_NOSEC handling is broken - we set it after each file_remove_suid() call.
>    However we needn't have removed suid bit simply because we have
>    CAP_SYS_FSID and further writes to the file from processes without this
>    capability still need to clear the suid bit.
> 2) file_remove_suid() is a misnomer since it also handles removing of
>    security labels. It is even more confusing because should_remove_suid()
>    doesn't return whether file_remove_suid() is needed or not.
> 3) On truncate we do clear suid bits but not security labels. According to
>    documentation in include/linux/security.h that's a bug but please correct
>    me if I'm wrong.
> 4) ocfs2 doesn't clear security labels - hard to fix, I left it alone for now.
> 5) XFS didn't provide proper exclusion for clearing mode bits.
> 
>   This series aims at fixing above issues.
> 
>   Since v1 I have removed bogus patch changing inode_set_flags(), I have
> updated changelog of patch 4/5 to better explain why ->inode_killpriv should
> be called and I have included a fix for MS_NOSEC handling in this series.
> Al, can you please merge the patches? Thanks!

Hi Al + Jan,

What's happening with this patchset? If it's not going to be pulled
into the VFS, I'll just pull in a version of the XFS patch that
corrects the locking at this point...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/5 v2 RESEND] fs: Fixes for removing xid bits and security labels

[Jan Kara]

On Tue 05-05-15 09:13:10, Dave Chinner wrote:
> On Tue, Mar 03, 2015 at 11:38:34AM +0100, Jan Kara wrote:
> >   Hello,
> > 
> >   warning in XFS made me look into detail into how clearing of suid / sgid
> > bits and security labels is done. And I've spotted a few issues:
> > 1) MS_NOSEC handling is broken - we set it after each file_remove_suid() call.
> >    However we needn't have removed suid bit simply because we have
> >    CAP_SYS_FSID and further writes to the file from processes without this
> >    capability still need to clear the suid bit.
> > 2) file_remove_suid() is a misnomer since it also handles removing of
> >    security labels. It is even more confusing because should_remove_suid()
> >    doesn't return whether file_remove_suid() is needed or not.
> > 3) On truncate we do clear suid bits but not security labels. According to
> >    documentation in include/linux/security.h that's a bug but please correct
> >    me if I'm wrong.
> > 4) ocfs2 doesn't clear security labels - hard to fix, I left it alone for now.
> > 5) XFS didn't provide proper exclusion for clearing mode bits.
> > 
> >   This series aims at fixing above issues.
> > 
> >   Since v1 I have removed bogus patch changing inode_set_flags(), I have
> > updated changelog of patch 4/5 to better explain why ->inode_killpriv should
> > be called and I have included a fix for MS_NOSEC handling in this series.
> > Al, can you please merge the patches? Thanks!
> 
> Hi Al + Jan,
> 
> What's happening with this patchset? If it's not going to be pulled
> into the VFS, I'll just pull in a version of the XFS patch that
> corrects the locking at this point...
  I've sent the patches several times but so far they were ignored. Al?

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR