Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[Amerigo Wang <amwang@redhat.com>]

OGAWA Hirofumi wrote:
> Amerigo Wang <amwang@redhat.com> writes:
>
>   
>> OGAWA Hirofumi wrote:
>>     
>>> akpm@linux-foundation.org writes:
>>>
>>>   
>>>       
>>>> diff -puN fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set fs/open.c
>>>> --- a/fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set
>>>> +++ a/fs/open.c
>>>> @@ -213,11 +213,15 @@ int do_truncate(struct dentry *dentry, l
>>>>  		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;
>>>>     
>>>>         
>>> Can't we use ATTR_FORCE for this? Because this calls notify_change()
>>> twice, and I guess this removes s[ug]id even if vmtruncate() (or in
>>> future ->truncate() may return error) or something returned error.
>>>
>>> I think it would not be good behavior.
>>>   
>>>       
>> Hi, please check:
>> http://lkml.org/lkml/2009/7/1/459
>>     
>
> Sorry for same argument. I see. However, um...
>
> I found this piece in security/selinux/hooks.c
>
> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
> {
> 	const struct cred *cred = current_cred();
>
> 	if (iattr->ia_valid & ATTR_FORCE)
> 		return 0;
>
> 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
> 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
> 		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>
> 	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
> }
>
> I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE),
> but truncate() already does it, I don't know whether it's ok. 

No, here we should only force ATTR_KILL_SUID and/or ATTR_KILL_SGID. 
do_truncate() has ATTR_SIZE and ATTR_FILE.