From mboxrd@z Thu Jan  1 00:00:00 1970
From: Amerigo Wang <amwang@redhat.com>
Subject: Re: [patch 12/12] vfs: allow file truncations when both suid and
 write permissions set
Date: Fri, 07 Aug 2009 17:27:48 +0800
Message-ID: <4A7BF394.8010302@redhat.com>
References: <200908062310.n76NAIEo013014@imap1.linux-foundation.org>	<87ocqs2w17.fsf@devron.myhome.or.jp> <4A7B9DC5.7080700@redhat.com> <871vno1cn7.fsf@devron.myhome.or.jp>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: akpm@linux-foundation.org, viro@zeniv.linux.org.uk,
	linux-fsdevel@vger.kernel.org, eparis@redhat.com,
	esandeen@redhat.com, eteo@redhat.com
To: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:55893 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753737AbZHGJ0C (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Fri, 7 Aug 2009 05:26:02 -0400
In-Reply-To: <871vno1cn7.fsf@devron.myhome.or.jp>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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.