All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org, xfs@oss.sgi.com, Jan Kara <jack@suse.cz>
Subject: [PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate
Date: Tue,  3 Mar 2015 11:38:38 +0100	[thread overview]
Message-ID: <1425379119-3773-5-git-send-email-jack@suse.cz> (raw)
In-Reply-To: <1425379119-3773-1-git-send-email-jack@suse.cz>

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


WARNING: multiple messages have this Message-ID (diff)
From: Jan Kara <jack@suse.cz>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org, Jan Kara <jack@suse.cz>, xfs@oss.sgi.com
Subject: [PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate
Date: Tue,  3 Mar 2015 11:38:38 +0100	[thread overview]
Message-ID: <1425379119-3773-5-git-send-email-jack@suse.cz> (raw)
In-Reply-To: <1425379119-3773-1-git-send-email-jack@suse.cz>

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

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

  parent reply	other threads:[~2015-03-03 10:38 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-03 10:38 [PATCH 0/5 v2 RESEND] fs: Fixes for removing xid bits and security labels Jan Kara
2015-03-03 10:38 ` Jan Kara
2015-03-03 10:38 ` [PATCH 1/5] fs: Fix S_NOSEC handling Jan Kara
2015-03-03 10:38   ` Jan Kara
2015-03-03 10:38 ` [PATCH 2/5] fs: Rename file_remove_suid() to file_remove_privs() Jan Kara
2015-03-03 10:38   ` Jan Kara
2015-03-03 10:38 ` [PATCH 3/5] fs: Provide function telling whether file_remove_privs() will do anything Jan Kara
2015-03-03 10:38   ` Jan Kara
2015-03-03 10:38 ` Jan Kara [this message]
2015-03-03 10:38   ` [PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate Jan Kara
2015-03-03 10:38 ` [PATCH 5/5] xfs: Correctly lock inode when removing suid and security marks Jan Kara
2015-03-03 10:38   ` Jan Kara
2015-03-03 21:34   ` Dave Chinner
2015-03-03 21:34     ` Dave Chinner
2015-04-14 16:54   ` Eric Sandeen
2015-04-14 16:54     ` Eric Sandeen
2015-04-14 23:03     ` Dave Chinner
2015-04-14 23:03       ` Dave Chinner
2015-05-04 23:13 ` [PATCH 0/5 v2 RESEND] fs: Fixes for removing xid bits and security labels Dave Chinner
2015-05-04 23:13   ` Dave Chinner
2015-05-05  7:56   ` Jan Kara
2015-05-05  7:56     ` Jan Kara
  -- strict thread matches above, loose matches on Subject: below --
2015-05-21 14:05 [PATCH 0/5 v4] " Jan Kara
2015-05-21 14:05 ` [PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate Jan Kara
2015-05-19  9:46 [PATCH 0/5 v3] fs: Fixes for removing xid bits and security labels Jan Kara
2015-05-19  9:46 ` [PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate Jan Kara
2014-12-18 12:49 [PATCH 0/5 v2] fs: Fixes for removing xid bits and security labels Jan Kara
2014-12-18 12:49 ` [PATCH 4/5] fs: Call security_ops->inode_killpriv on truncate Jan Kara
2014-12-18 12:49   ` Jan Kara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1425379119-3773-5-git-send-email-jack@suse.cz \
    --to=jack@suse.cz \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=viro@ZenIV.linux.org.uk \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.