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

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Amerigo Wang <amwang@redhat.com> writes:

> 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.

As far as I can see,

do_truncate pass ATTR_SIZE
	security_inode_setattr()
		dentry_has_perm(cred, NULL, dentry, FILE__WRITE);

do_truncate pass (ATTR_SIZE | ATTR_MODE) [ATTR_MODE is set by ATTR_KILL_*.
                                          This should works fine for
	                                  file owner without patch.]
	security_inode_setattr()
		dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);

Doesn't security module really have to call dentry_has_perm() with
FILE__WRITE if (ATTR_SIZE | ATTR_MODE)?

> But clearly ATTR_FORCE is the way to bypass the security module on
> purpose.

Even if it's true, why don't we change security modules? I'm saying
always bypass was wrong way, if it doesn't want. AFAIK, it's not binary
driver, and it has good source codes.

> I agree that we perhaps should have some wrapper function to do this
> (instead of calling notify_change() twice), but currently this is
> fine.

I don't object to that patch though, but it's fine? Really? The error
handling is wrong, and note to call fs-driver twice means it can be
cause of I/O (network, storage) twice, fsnotify() is also called twice,
and fs-driver can't fix those.

I don't think those are fine at all.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>