From mboxrd@z Thu Jan 1 00:00:00 1970 From: Edward Shishkin Subject: Re: [PATCH #3] reiserfs: Fix permissions on .reiserfs_priv Date: Thu, 08 Apr 2010 23:38:42 +0200 Message-ID: <4BBE4CE2.8030805@gmail.com> References: <4BBE42A9.2010506@suse.com> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4BBE42A9.2010506@suse.com> Sender: reiserfs-devel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii"; format="flowed" To: Andrew Morton Cc: Jeff Mahoney , ReiserFS Mailing List , Linux Kernel Mailing List , matt@mattmccutchen.net Acked-by: Edward Shishkin Andrew, I think it should be applied ASAP. Thanks to Matt McCutchen who discovered this issue. Edward. Jeff Mahoney wrote: > Commit 677c9b2e393a0cd203bd54e9c18b012b2c73305a removed the magic > from the lookup code to hide the .reiserfs_priv directory since it > was getting loaded at mount-time instead. The intent was that the > entry would be hidden from the user via a poisoned d_compare, but > this was faulty. > > This introduced a security issue where unpriviledged users could > access and modify extended attributes or ACLs belonging to other > users, including root. > > This patch resolves the issue by properly hiding .reiserfs_priv. This > was the intent of the xattr poisoning code, but it appears to have > never worked as expected. This is fixed by using d_revalidate instead > of d_compare. > > This patch makes -oexpose_privroot a no-op. I'm fine leaving it this > way. The effort involved in working out the corner cases wrt permissions > and caching outweigh the benefit of the feature. > > Signed-off-by: Jeff Mahoney > --- > > fs/reiserfs/dir.c | 2 -- > fs/reiserfs/xattr.c | 17 ++++------------- > 2 files changed, 4 insertions(+), 15 deletions(-) > > --- a/fs/reiserfs/dir.c > +++ b/fs/reiserfs/dir.c > @@ -45,8 +45,6 @@ static inline bool is_privroot_deh(struc > struct reiserfs_de_head *deh) > { > struct dentry *privroot = REISERFS_SB(dir->d_sb)->priv_root; > - if (reiserfs_expose_privroot(dir->d_sb)) > - return 0; > return (dir == dir->d_parent && privroot->d_inode && > deh->deh_objectid == INODE_PKEY(privroot->d_inode)->k_objectid); > } > --- a/fs/reiserfs/xattr.c > +++ b/fs/reiserfs/xattr.c > @@ -972,21 +972,13 @@ int reiserfs_permission(struct inode *in > return generic_permission(inode, mask, NULL); > } > > -/* This will catch lookups from the fs root to .reiserfs_priv */ > -static int > -xattr_lookup_poison(struct dentry *dentry, struct qstr *q1, struct qstr *name) > +static int xattr_hide_revalidate(struct dentry *dentry, struct nameidata *nd) > { > - struct dentry *priv_root = REISERFS_SB(dentry->d_sb)->priv_root; > - if (container_of(q1, struct dentry, d_name) == priv_root) > - return -ENOENT; > - if (q1->len == name->len && > - !memcmp(q1->name, name->name, name->len)) > - return 0; > - return 1; > + return -EPERM; > } > > static const struct dentry_operations xattr_lookup_poison_ops = { > - .d_compare = xattr_lookup_poison, > + .d_revalidate = xattr_hide_revalidate, > }; > > int reiserfs_lookup_privroot(struct super_block *s) > @@ -1000,8 +992,7 @@ int reiserfs_lookup_privroot(struct supe > strlen(PRIVROOT_NAME)); > if (!IS_ERR(dentry)) { > REISERFS_SB(s)->priv_root = dentry; > - if (!reiserfs_expose_privroot(s)) > - s->s_root->d_op = &xattr_lookup_poison_ops; > + dentry->d_op = &xattr_lookup_poison_ops; > if (dentry->d_inode) > dentry->d_inode->i_flags |= S_PRIVATE; > } else > >