diff mbox

[v2] NFSD: Don't clear SUID/SGID after root writing data

Message ID 5351501B.1010507@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kinglong Mee April 18, 2014, 4:17 p.m. UTC 
SUID/SGID is not cleared after writing data with (root, local ext4),
#touch test; chmod 4777 test; stat test; echo 1234 > test; stat test;
   File: ‘test’
   Size: 0               Blocks: 0          IO Block: 4096   regular 
empty file
Device: 803h/2051d      Inode: 1200137     Links: 1
Access: (4777/-rwsrwxrwx)  Uid: (    0/    root)   Gid: (    0/    root)
Context: unconfined_u:object_r:admin_home_t:s0
Access: 2014-04-18 21:36:31.016029014 +0800
Modify: 2014-04-18 21:36:31.016029014 +0800
Change: 2014-04-18 21:36:31.026030285 +0800
  Birth: -
   File: ‘test’
   Size: 5               Blocks: 8          IO Block: 4096   regular file
Device: 803h/2051d      Inode: 1200137     Links: 1
Access: (4777/-rwsrwxrwx)  Uid: (    0/    root)   Gid: (    0/    root)
Context: unconfined_u:object_r:admin_home_t:s0
Access: 2014-04-18 21:36:31.016029014 +0800
Modify: 2014-04-18 21:36:31.040032065 +0800
Change: 2014-04-18 21:36:31.040032065 +0800
  Birth: -

With no_root_squash, (root, remote ext4), SUID/SGID are cleared,
#touch test; chmod 4777 test; stat test; echo 1234 > test; stat test;
   File: ‘test’
   Size: 0               Blocks: 0          IO Block: 262144 regular 
empty file
Device: 24h/36d Inode: 786439      Links: 1
Access: (4777/-rwsrwxrwx)  Uid: ( 1000/    test)   Gid: ( 1000/    test)
Context: system_u:object_r:nfs_t:s0
Access: 2014-04-18 21:45:32.155805097 +0800
Modify: 2014-04-18 21:45:32.155805097 +0800
Change: 2014-04-18 21:45:32.168806749 +0800
  Birth: -
   File: ‘test’
   Size: 5               Blocks: 8          IO Block: 262144 regular file
Device: 24h/36d Inode: 786439      Links: 1
Access: (0777/-rwxrwxrwx)  Uid: ( 1000/    test)   Gid: ( 1000/    test)
Context: system_u:object_r:nfs_t:s0
Access: 2014-04-18 21:45:32.155805097 +0800
Modify: 2014-04-18 21:45:32.184808783 +0800
Change: 2014-04-18 21:45:32.184808783 +0800
  Birth: -

This patch drops codes that kills SGID/SUID visibly,
because vfs_writev() can kills it if necessary.

With this patch, the above problem will not exist.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
  fs/nfsd/vfs.c | 18 ------------------
  1 file changed, 18 deletions(-)

   * there's a high chance this is another nfsd (triggered by a bulk write
@@ -942,10 +928,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct 
svc_fh *fhp, struct file *file,
  	nfsdstats.io_write += host_err;
  	fsnotify_modify(file);

-	/* clear setuid/setgid flag after write */
-	if (inode->i_mode & (S_ISUID | S_ISGID))
-		kill_suid(dentry);
-
  	if (stable) {
  		if (use_wgather)
  			host_err = wait_for_concurrent_writes(file);

Comments

J. Bruce Fields May 8, 2014, 4:12 p.m. UTC | #1 
On Sat, Apr 19, 2014 at 12:17:31AM +0800, Kinglong Mee wrote:
> SUID/SGID is not cleared after writing data with (root, local ext4),
> #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test;
>   File: ‘test’
>   Size: 0               Blocks: 0          IO Block: 4096   regular
> empty file
> Device: 803h/2051d      Inode: 1200137     Links: 1
> Access: (4777/-rwsrwxrwx)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: unconfined_u:object_r:admin_home_t:s0
> Access: 2014-04-18 21:36:31.016029014 +0800
> Modify: 2014-04-18 21:36:31.016029014 +0800
> Change: 2014-04-18 21:36:31.026030285 +0800
>  Birth: -
>   File: ‘test’
>   Size: 5               Blocks: 8          IO Block: 4096   regular file
> Device: 803h/2051d      Inode: 1200137     Links: 1
> Access: (4777/-rwsrwxrwx)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: unconfined_u:object_r:admin_home_t:s0
> Access: 2014-04-18 21:36:31.016029014 +0800
> Modify: 2014-04-18 21:36:31.040032065 +0800
> Change: 2014-04-18 21:36:31.040032065 +0800
>  Birth: -
> 
> With no_root_squash, (root, remote ext4), SUID/SGID are cleared,
> #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test;
>   File: ‘test’
>   Size: 0               Blocks: 0          IO Block: 262144 regular
> empty file
> Device: 24h/36d Inode: 786439      Links: 1
> Access: (4777/-rwsrwxrwx)  Uid: ( 1000/    test)   Gid: ( 1000/    test)
> Context: system_u:object_r:nfs_t:s0
> Access: 2014-04-18 21:45:32.155805097 +0800
> Modify: 2014-04-18 21:45:32.155805097 +0800
> Change: 2014-04-18 21:45:32.168806749 +0800
>  Birth: -
>   File: ‘test’
>   Size: 5               Blocks: 8          IO Block: 262144 regular file
> Device: 24h/36d Inode: 786439      Links: 1
> Access: (0777/-rwxrwxrwx)  Uid: ( 1000/    test)   Gid: ( 1000/    test)
> Context: system_u:object_r:nfs_t:s0
> Access: 2014-04-18 21:45:32.155805097 +0800
> Modify: 2014-04-18 21:45:32.184808783 +0800
> Change: 2014-04-18 21:45:32.184808783 +0800
>  Birth: -
> 
> This patch drops codes that kills SGID/SUID visibly,
> because vfs_writev() can kills it if necessary.
> 
> With this patch, the above problem will not exist.

I'd like to apply this if only to remove the redundant code.

I'd like to understand, though, whether this is something that caused an
actual practical problem for someone, or if you just happened to notice
the inconsistency between nfs and ext4 behavior?

--b.

> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/vfs.c | 18 ------------------
>  1 file changed, 18 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 16f0673..6aaa305 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -857,20 +857,6 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct
> svc_fh *fhp, struct file *file,
>  	return err;
>  }
> 
> -static void kill_suid(struct dentry *dentry)
> -{
> -	struct iattr	ia;
> -	ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> -
> -	mutex_lock(&dentry->d_inode->i_mutex);
> -	/*
> -	 * Note we call this on write, so notify_change will not
> -	 * encounter any conflicting delegations:
> -	 */
> -	notify_change(dentry, &ia, NULL);
> -	mutex_unlock(&dentry->d_inode->i_mutex);
> -}
> -
>  /*
>   * Gathered writes: If another process is currently writing to the file,
>   * there's a high chance this is another nfsd (triggered by a bulk write
> @@ -942,10 +928,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct
> svc_fh *fhp, struct file *file,
>  	nfsdstats.io_write += host_err;
>  	fsnotify_modify(file);
> 
> -	/* clear setuid/setgid flag after write */
> -	if (inode->i_mode & (S_ISUID | S_ISGID))
> -		kill_suid(dentry);
> -
>  	if (stable) {
>  		if (use_wgather)
>  			host_err = wait_for_concurrent_writes(file);
> -- 
> 1.9.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kinglong Mee May 9, 2014, 7:55 a.m. UTC | #2 
On 5/9/2014 00:12, J. Bruce Fields wrote:
> On Sat, Apr 19, 2014 at 12:17:31AM +0800, Kinglong Mee wrote:
>> SUID/SGID is not cleared after writing data with (root, local ext4),
>> #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test;
>>   File: ‘test’
>>   Size: 0               Blocks: 0          IO Block: 4096   regular
>> empty file
>> Device: 803h/2051d      Inode: 1200137     Links: 1
>> Access: (4777/-rwsrwxrwx)  Uid: (    0/    root)   Gid: (    0/    root)
>> Context: unconfined_u:object_r:admin_home_t:s0
>> Access: 2014-04-18 21:36:31.016029014 +0800
>> Modify: 2014-04-18 21:36:31.016029014 +0800
>> Change: 2014-04-18 21:36:31.026030285 +0800
>>  Birth: -
>>   File: ‘test’
>>   Size: 5               Blocks: 8          IO Block: 4096   regular file
>> Device: 803h/2051d      Inode: 1200137     Links: 1
>> Access: (4777/-rwsrwxrwx)  Uid: (    0/    root)   Gid: (    0/    root)
>> Context: unconfined_u:object_r:admin_home_t:s0
>> Access: 2014-04-18 21:36:31.016029014 +0800
>> Modify: 2014-04-18 21:36:31.040032065 +0800
>> Change: 2014-04-18 21:36:31.040032065 +0800
>>  Birth: -
>>
>> With no_root_squash, (root, remote ext4), SUID/SGID are cleared,
>> #touch test; chmod 4777 test; stat test; echo 1234 > test; stat test;
>>   File: ‘test’
>>   Size: 0               Blocks: 0          IO Block: 262144 regular
>> empty file
>> Device: 24h/36d Inode: 786439      Links: 1
>> Access: (4777/-rwsrwxrwx)  Uid: ( 1000/    test)   Gid: ( 1000/    test)
>> Context: system_u:object_r:nfs_t:s0
>> Access: 2014-04-18 21:45:32.155805097 +0800
>> Modify: 2014-04-18 21:45:32.155805097 +0800
>> Change: 2014-04-18 21:45:32.168806749 +0800
>>  Birth: -
>>   File: ‘test’
>>   Size: 5               Blocks: 8          IO Block: 262144 regular file
>> Device: 24h/36d Inode: 786439      Links: 1
>> Access: (0777/-rwxrwxrwx)  Uid: ( 1000/    test)   Gid: ( 1000/    test)
>> Context: system_u:object_r:nfs_t:s0
>> Access: 2014-04-18 21:45:32.155805097 +0800
>> Modify: 2014-04-18 21:45:32.184808783 +0800
>> Change: 2014-04-18 21:45:32.184808783 +0800
>>  Birth: -
>>
>> This patch drops codes that kills SGID/SUID visibly,
>> because vfs_writev() can kills it if necessary.
>>
>> With this patch, the above problem will not exist.
> 
> I'd like to apply this if only to remove the redundant code.
> 
> I'd like to understand, though, whether this is something that caused an
> actual practical problem for someone, or if you just happened to notice
> the inconsistency between nfs and ext4 behavior?

I test it with ext2,ext3,btrfs,xfs.
Test result is same as ext4.
So, we needs remove the redundant killing of suid/sgid.

thanks,
Kinglong Mee

> 
> --b.
> 
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/nfsd/vfs.c | 18 ------------------
>>  1 file changed, 18 deletions(-)
>>
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index 16f0673..6aaa305 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -857,20 +857,6 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct
>> svc_fh *fhp, struct file *file,
>>  	return err;
>>  }
>>
>> -static void kill_suid(struct dentry *dentry)
>> -{
>> -	struct iattr	ia;
>> -	ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
>> -
>> -	mutex_lock(&dentry->d_inode->i_mutex);
>> -	/*
>> -	 * Note we call this on write, so notify_change will not
>> -	 * encounter any conflicting delegations:
>> -	 */
>> -	notify_change(dentry, &ia, NULL);
>> -	mutex_unlock(&dentry->d_inode->i_mutex);
>> -}
>> -
>>  /*
>>   * Gathered writes: If another process is currently writing to the file,
>>   * there's a high chance this is another nfsd (triggered by a bulk write
>> @@ -942,10 +928,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct
>> svc_fh *fhp, struct file *file,
>>  	nfsdstats.io_write += host_err;
>>  	fsnotify_modify(file);
>>
>> -	/* clear setuid/setgid flag after write */
>> -	if (inode->i_mode & (S_ISUID | S_ISGID))
>> -		kill_suid(dentry);
>> -
>>  	if (stable) {
>>  		if (use_wgather)
>>  			host_err = wait_for_concurrent_writes(file);
>> -- 
>> 1.9.0
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 16f0673..6aaa305 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -857,20 +857,6 @@  nfsd_vfs_read(struct svc_rqst *rqstp, struct svc_fh 
*fhp, struct file *file,
  	return err;
  }

-static void kill_suid(struct dentry *dentry)
-{
-	struct iattr	ia;
-	ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
-
-	mutex_lock(&dentry->d_inode->i_mutex);
-	/*
-	 * Note we call this on write, so notify_change will not
-	 * encounter any conflicting delegations:
-	 */
-	notify_change(dentry, &ia, NULL);
-	mutex_unlock(&dentry->d_inode->i_mutex);
-}
-
  /*
   * Gathered writes: If another process is currently writing to the file,