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

[Amerigo Wang <amwang@redhat.com>]

OGAWA Hirofumi wrote:
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>
>   
>> Amerigo Wang <amwang@redhat.com> writes:
>>
>>     
>>>> 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.
>>>       
>> I guess security module should do,
>>
>> 	ia_valid = iattr->ia_valid;
>> 	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
>> 		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>> 		if (err)
>>                 	return err;
>> 		ia_valid &= ~ATTR_FORCE_MASK;
>> 	}
>> 	if (ia_valid & ATTR_NOT_FORCE_MASK)
>> 	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
>> 	return err;
>>
>> or something. Because do_truncate() already do (ATTR_MODE | ATTR_SIZE)
>> without ATTR_FORCE.
>>     
>
> BTW, it seems original code doesn't check ATTR_SIZE if (ATTR_MODE |
> ATTR_SIZE), right?  So, ATTR_FORCE is just forcing ATTR_MODE, but I
> guess that's problem itself.
>   

I am not sure if I understand you correctly... You must be referring 
notify_change(), it seems to do what you said.

But clearly ATTR_FORCE is the way to bypass the security module on 
purpose. I agree that we perhaps should have some wrapper function to do 
this (instead of calling notify_change() twice), but currently this is fine.