From mboxrd@z Thu Jan 1 00:00:00 1970 From: OGAWA Hirofumi Subject: Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set Date: Mon, 10 Aug 2009 13:34:03 +0900 Message-ID: <87fxc070ec.fsf@devron.myhome.or.jp> 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> <4A7F7F2E.5070807@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: Amerigo Wang Return-path: Received: from mail.parknet.ad.jp ([210.171.162.6]:37012 "EHLO mail.officemail.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750943AbZHJEeI (ORCPT ); Mon, 10 Aug 2009 00:34:08 -0400 In-Reply-To: <4A7F7F2E.5070807@redhat.com> (Amerigo Wang's message of "Mon, 10 Aug 2009 10:00:14 +0800") Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Amerigo Wang writes: > OGAWA Hirofumi wrote: >> OGAWA Hirofumi writes: >> >> >>> Amerigo Wang 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