allow file truncations when both suid and write permissions set

PATCH RESEND Superseded Amerigo Wang 2 participants 2 messages Jul 22, 2009 – Jul 24, 2009

Subsystems (3)

Memory management VFS Page cache

lore

Share & tools

lore

Amerigo Wang

author

2009-07-22 09:18 UTC #0 lore raw

PATCH RESEND v2 1/1 allow file truncations when both suid and write permissions set

V1 -> V2:
Introduce dentry_remove_suid(), and use it in do_truncate(). Thanks to Eric Paris.

When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.

However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.

Steps to reproduce this bug:

% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h > rootdir/file1
zsh: operation not permitted: rootdir/file1 % ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h >> rootdir/file1
% ls -l rootdir/file1
-rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1

This patch fixes it.

Signed-off-by WANG Cong <[email protected]>

Cc [email protected]

Cc Eric Sandeen <[email protected]>

Cc Eric Paris <[email protected]>

Cc Eugene Teo <[email protected]>

Cc Al Viro <[email protected]>

---

fs/open.c +6 -2

index dd98e80..ed5cd45 100644
@@ -213,11 +213,15 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 		newattrs.ia_valid |= ATTR_FILE;
 	}
 
+	mutex_lock(&dentry->d_inode->i_mutex);
 	/* Remove suid/sgid on truncate too */
-	newattrs.ia_valid |= should_remove_suid(dentry);
+	err = dentry_remove_suid(dentry);
+	if (err)
+		goto unlock;
 
-	mutex_lock(&dentry->d_inode->i_mutex);
 	err = notify_change(dentry, &newattrs);
+
+ unlock:
 	mutex_unlock(&dentry->d_inode->i_mutex);
 	return err;
 }

include/linux/fs.h +1 -0

index 0872372..8baf558 100644
@@ -2166,6 +2166,7 @@ extern void destroy_inode(struct inode *);
 extern struct inode *new_inode(struct super_block *);
 extern int should_remove_suid(struct dentry *);
 extern int file_remove_suid(struct file *);
+extern int dentry_remove_suid(struct dentry *);
 
 extern void __insert_inode_hash(struct inode *, unsigned long hashval);
 extern void remove_inode_hash(struct inode *);

mm/filemap.c +10 -2

index 2239671..b07f7d0 100644
@@ -1839,9 +1839,11 @@ static int __remove_suid(struct dentry *dentry, int kill)
 	return notify_change(dentry, &newattrs);
 }
 
-int file_remove_suid(struct file *file)
+/*
+ * Note: you need to hold i_mutex before call this.
+ */
+int dentry_remove_suid(struct dentry *dentry)
 {
-	struct dentry *dentry = file->f_path.dentry;
 	int killsuid = should_remove_suid(dentry);
 	int killpriv = security_inode_need_killpriv(dentry);
 	int error = 0;

@@ -1855,6 +1857,12 @@ int file_remove_suid(struct file *file)
 
 	return error;
 }
+
+int file_remove_suid(struct file *file)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	return dentry_remove_suid(dentry);
+}
 EXPORT_SYMBOL(file_remove_suid);
 
 static size_t __iovec_copy_from_user_inatomic(char *vaddr,

Andrew Morton

maintainer

2009-07-24 21:35 UTC #1 lore raw

Quoted 25 lines · On Wed, 22 Jul 2009 05:17:02 -0400 Amerigo Wang <[email protected]> wrote:

V1 -> V2:
Introduce dentry_remove_suid(), and use it in do_truncate(). Thanks to Eric Paris.
When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.
However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.
Steps to reproduce this bug:
% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h > rootdir/file1
zsh: operation not permitted: rootdir/file1 % ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h >> rootdir/file1
% ls -l rootdir/file1
-rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1

Quoted 1 line

This patch fixes it.

Thanks, I queued this up for Al's consideration.

Quoted 6 lines

Signed-off-by WANG Cong <[email protected]>
Cc [email protected]
Cc Eric Sandeen <[email protected]>
Cc Eric Paris <[email protected]>
Cc Eugene Teo <[email protected]>
Cc Al Viro <[email protected]>

I removed the Cc:stable. I'm not really sure whether we want to backport this.

In fact I'm not really sure that we want to merge it at all. It introduces the risk that people will develope, test and ship applications which malfunction when run on older kernels.

otoh we already have the risk that people's applications will run OK on other OS's and will malfunction on Linux, and the patch fixes this.