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:
>> 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. The
definition of ATTR_FORCE is unclear at all, it would be problem. But,
I'm not sure though, I suspect the above code also has problem...

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