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

[PATCH 0/5] 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) 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.
2) on truncate we do clear suid bits but not security labels. I think that's
   a bug but please correct me if I'm wrong.
3) ocfs2 doesn't clear security labels - hard to fix, I left it alone for now.
4) XFS didn't provide proper exclusion for clearing mode bits.

  This series aims at fixing above issues. The second patch in the series
is unrelated fix to inode_set_mask() which I spotted when playing with the
code.

								Honza

[PATCH 1/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 +-
 mm/filemap_xip.c   |  2 +-
 9 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index a18ceabd99a8..123795de7103 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1752,7 +1752,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 d7e0da8366e6..566c04d7ce68 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -901,7 +901,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 caa8d95b24e8..dca9311e628f 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1214,7 +1214,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 f5e01704a5c8..fcc0886c5824 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1610,7 +1610,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;
@@ -1637,7 +1641,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
@@ -1906,9 +1910,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 643faa44f22b..d9b8f3d953a9 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -2098,7 +2098,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 eb596b419942..5c9e8296ebb3 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -557,7 +557,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 9ab779e8a63c..96b2d6a9a6cb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2428,7 +2428,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 14b4642279f1..da23d6fc102c 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2580,7 +2580,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;
 
diff --git a/mm/filemap_xip.c b/mm/filemap_xip.c
index d8d9fe3f685c..8c0425b20b8a 100644
--- a/mm/filemap_xip.c
+++ b/mm/filemap_xip.c
@@ -423,7 +423,7 @@ xip_file_write(struct file *filp, const char __user *buf, size_t len,
 	if (count == 0)
 		goto out_backing;
 
-	ret = file_remove_suid(filp);
+	ret = file_remove_privs(filp);
 	if (ret)
 		goto out_backing;
 
-- 
1.8.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/5] fs: Fix WARN_ON in inode_set_mask()

[Jan Kara]

WARN_ON in inode_set_mask() warns if we don't clear all bits we are
setting instead of reverse - warning when caller requests setting and
clearing of the same bit.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index fcc0886c5824..3490389dc813 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1923,7 +1923,7 @@ void inode_set_flags(struct inode *inode, unsigned int flags,
 {
 	unsigned int old_flags, new_flags;
 
-	WARN_ON_ONCE(flags & ~mask);
+	WARN_ON_ONCE(flags & mask);
 	do {
 		old_flags = ACCESS_ONCE(inode->i_flags);
 		new_flags = (old_flags & ~mask) | flags;
-- 
1.8.1.4

Re: [PATCH 2/5] fs: Fix WARN_ON in inode_set_mask()

[Al Viro]

On Thu, Dec 04, 2014 at 02:27:36PM +0100, Jan Kara wrote:
> WARN_ON in inode_set_mask() warns if we don't clear all bits we are
> setting instead of reverse - warning when caller requests setting and
> clearing of the same bit.

WTF?  'mask' is "all bits we are asked to modify", 'flags' - "the values to
put into those bits".  Have you even looked at the callers, let alone
tested it?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/5] fs: Fix WARN_ON in inode_set_mask()

[Jan Kara]

On Thu 04-12-14 14:37:09, Al Viro wrote:
> On Thu, Dec 04, 2014 at 02:27:36PM +0100, Jan Kara wrote:
> > WARN_ON in inode_set_mask() warns if we don't clear all bits we are
> > setting instead of reverse - warning when caller requests setting and
> > clearing of the same bit.
> 
> WTF?  'mask' is "all bits we are asked to modify", 'flags' - "the values to
> put into those bits".  Have you even looked at the callers, let alone
> tested it?
  Sorry, I misunderstood how the function is supposed to work. Discard this
patch please.

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

[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 3490389dc813..6807a2707828 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1598,7 +1598,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;
 
@@ -1618,23 +1643,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 96b2d6a9a6cb..aac707cced66 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2429,6 +2429,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)
-- 
1.8.1.4

[PATCH 4/5] fs: Remove security attributes on truncate

[Jan Kara]

Similarly as we remove suid bit on truncate, we also want to remove
security extended attributes.

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 6807a2707828..8595c7b8841c 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1603,9 +1603,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;
@@ -1621,7 +1620,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 de92c13b58be..e4e0863855d0 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 aac707cced66..c5ccc311e8fb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2429,7 +2429,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)
-- 
1.8.1.4

Re: [PATCH 4/5] fs: Remove security attributes on truncate

[Casey Schaufler]

On 12/4/2014 5:27 AM, Jan Kara wrote:
> Similarly as we remove suid bit on truncate, we also want to remove
> security extended attributes.

NAK

Are you out of your mind?

In Smack and SELinux the security attributes are associated with the
container, not the data.

>
> 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 6807a2707828..8595c7b8841c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1603,9 +1603,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;
> @@ -1621,7 +1620,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 de92c13b58be..e4e0863855d0 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 aac707cced66..c5ccc311e8fb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2429,7 +2429,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)

Re: [PATCH 4/5] fs: Remove security attributes on truncate

[Jan Kara]

On Fri 05-12-14 08:06:55, Casey Schaufler wrote:
> On 12/4/2014 5:27 AM, Jan Kara wrote:
> > Similarly as we remove suid bit on truncate, we also want to remove
> > security extended attributes.
> 
> NAK
> 
> Are you out of your mind?
>
> In Smack and SELinux the security attributes are associated with the
> container, not the data.
  Is there some doc for this? It just seems strange to me that when a file
is written we clear the attributes but when the file is truncated we don't.
What's the rationale behind this? To me both operations modify content of
the file and thus I'd expect them to behave identically with respect to
security attributes...

								Honza

> > 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 6807a2707828..8595c7b8841c 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -1603,9 +1603,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;
> > @@ -1621,7 +1620,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 de92c13b58be..e4e0863855d0 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 aac707cced66..c5ccc311e8fb 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2429,7 +2429,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)
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 4/5] fs: Remove security attributes on truncate

[Casey Schaufler]

On 12/9/2014 10:27 AM, Jan Kara wrote:
> On Fri 05-12-14 08:06:55, Casey Schaufler wrote:
>> On 12/4/2014 5:27 AM, Jan Kara wrote:
>>> Similarly as we remove suid bit on truncate, we also want to remove
>>> security extended attributes.
>> NAK
>>
>> Are you out of your mind?
>>
>> In Smack and SELinux the security attributes are associated with the
>> container, not the data.
>   Is there some doc for this? It just seems strange to me that when a file
> is written we clear the attributes

This is not true for the LSM based attributes.

>  but when the file is truncated we don't.

Have I miss-interpreted what you meant by "security extended attributes"?
Do you mean filesystem xattrs beginning with "security.", such as
"security.selinux" or "security.SMACK64", or something else?

> What's the rationale behind this? To me both operations modify content of
> the file and thus I'd expect them to behave identically with respect to
> security attributes...
>
> 								Honza
>
>>> 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 6807a2707828..8595c7b8841c 100644
>>> --- a/fs/inode.c
>>> +++ b/fs/inode.c
>>> @@ -1603,9 +1603,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;
>>> @@ -1621,7 +1620,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 de92c13b58be..e4e0863855d0 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 aac707cced66..c5ccc311e8fb 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -2429,7 +2429,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)

Re: [PATCH 4/5] fs: Remove security attributes on truncate

[Jan Kara]

On Tue 09-12-14 10:59:29, Casey Schaufler wrote:
> On 12/9/2014 10:27 AM, Jan Kara wrote:
> > On Fri 05-12-14 08:06:55, Casey Schaufler wrote:
> >> On 12/4/2014 5:27 AM, Jan Kara wrote:
> >>> Similarly as we remove suid bit on truncate, we also want to remove
> >>> security extended attributes.
> >> NAK
> >>
> >> Are you out of your mind?
> >>
> >> In Smack and SELinux the security attributes are associated with the
> >> container, not the data.
> >   Is there some doc for this? It just seems strange to me that when a file
> > is written we clear the attributes
> 
> This is not true for the LSM based attributes.
> 
> >  but when the file is truncated we don't.
> 
> Have I miss-interpreted what you meant by "security extended attributes"?
> Do you mean filesystem xattrs beginning with "security.", such as
> "security.selinux" or "security.SMACK64", or something else?
  Sorry, I'm not a security guy so I may be using wrong terminology. I
meant attributes that are removed when you call security_inode_killpriv().
There's a comment in security.h like:
 * @inode_killpriv:
 *      The setuid bit is being removed.  Remove similar security labels.
 *      Called with the dentry->d_inode->i_mutex held.
 *      @dentry is the dentry being changed.
 *      Return 0 on success.  If error is returned, then the operation
 *      causing setuid bit removal is failed.

So from that I'd think that security_inode_killpriv() should be called if
we are removing SUID bit (i.e. also during truncate).

								Honza

> > What's the rationale behind this? To me both operations modify content of
> > the file and thus I'd expect them to behave identically with respect to
> > security attributes...
> >
> > 								Honza
> >
> >>> 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 6807a2707828..8595c7b8841c 100644
> >>> --- a/fs/inode.c
> >>> +++ b/fs/inode.c
> >>> @@ -1603,9 +1603,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;
> >>> @@ -1621,7 +1620,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 de92c13b58be..e4e0863855d0 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 aac707cced66..c5ccc311e8fb 100644
> >>> --- a/include/linux/fs.h
> >>> +++ b/include/linux/fs.h
> >>> @@ -2429,7 +2429,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)
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 4/5] fs: Remove security attributes on truncate

[Jan Kara]

On Wed 10-12-14 12:11:23, Jan Kara wrote:
> On Tue 09-12-14 10:59:29, Casey Schaufler wrote:
> > On 12/9/2014 10:27 AM, Jan Kara wrote:
> > > On Fri 05-12-14 08:06:55, Casey Schaufler wrote:
> > >> On 12/4/2014 5:27 AM, Jan Kara wrote:
> > >>> Similarly as we remove suid bit on truncate, we also want to remove
> > >>> security extended attributes.
> > >> NAK
> > >>
> > >> Are you out of your mind?
> > >>
> > >> In Smack and SELinux the security attributes are associated with the
> > >> container, not the data.
> > >   Is there some doc for this? It just seems strange to me that when a file
> > > is written we clear the attributes
> > 
> > This is not true for the LSM based attributes.
> > 
> > >  but when the file is truncated we don't.
> > 
> > Have I miss-interpreted what you meant by "security extended attributes"?
> > Do you mean filesystem xattrs beginning with "security.", such as
> > "security.selinux" or "security.SMACK64", or something else?
>   Sorry, I'm not a security guy so I may be using wrong terminology. I
> meant attributes that are removed when you call security_inode_killpriv().
> There's a comment in security.h like:
>  * @inode_killpriv:
>  *      The setuid bit is being removed.  Remove similar security labels.
>  *      Called with the dentry->d_inode->i_mutex held.
>  *      @dentry is the dentry being changed.
>  *      Return 0 on success.  If error is returned, then the operation
>  *      causing setuid bit removal is failed.
> 
> So from that I'd think that security_inode_killpriv() should be called if
> we are removing SUID bit (i.e. also during truncate).
  Casey, so are you OK which this change?

								Honza

> > > What's the rationale behind this? To me both operations modify content of
> > > the file and thus I'd expect them to behave identically with respect to
> > > security attributes...
> > >
> > > 								Honza
> > >
> > >>> 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 6807a2707828..8595c7b8841c 100644
> > >>> --- a/fs/inode.c
> > >>> +++ b/fs/inode.c
> > >>> @@ -1603,9 +1603,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;
> > >>> @@ -1621,7 +1620,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 de92c13b58be..e4e0863855d0 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 aac707cced66..c5ccc311e8fb 100644
> > >>> --- a/include/linux/fs.h
> > >>> +++ b/include/linux/fs.h
> > >>> @@ -2429,7 +2429,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)
> > 
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[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 5c9e8296ebb3..c6622aa1f8af 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -521,6 +521,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
@@ -557,7 +564,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;
 }
 
 /*
-- 
1.8.1.4