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: Fri, 07 Aug 2009 13:17:00 +0900 Message-ID: <871vno1cn7.fsf@devron.myhome.or.jp> References: <200908062310.n76NAIEo013014@imap1.linux-foundation.org> <87ocqs2w17.fsf@devron.myhome.or.jp> <4A7B9DC5.7080700@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]:57779 "EHLO mail.officemail.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752273AbZHGERD (ORCPT ); Fri, 7 Aug 2009 00:17:03 -0400 In-Reply-To: <4A7B9DC5.7080700@redhat.com> (Amerigo Wang's message of "Fri, 07 Aug 2009 11:21:41 +0800") Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Amerigo Wang 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