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: Mon, 10 Aug 2009 10:00:14 +0800
Message-ID: <4A7F7F2E.5070807@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> <4A7BF394.8010302@redhat.com>	<87prb7sx8p.fsf@devron.myhome.or.jp> <87ab2bsw6n.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]:35686 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754545AbZHJB6C (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Sun, 9 Aug 2009 21:58:02 -0400
In-Reply-To: <87ab2bsw6n.fsf@devron.myhome.or.jp>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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.