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

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

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.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>