linux-kernel - Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 04 May 2009 01:01:17 -0400
From:	Jeff Mahoney <jeffm@...e.com>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	ReiserFS Mailing List <reiserfs-devel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@....linux.org.uk>,
	Alexander Beregalov <a.beregalov@...il.com>,
	David <david@...olicited.net>
Subject: Re: [PATCH] reiserfs: Expand i_mutex to enclose lookup_one_len

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Al Viro wrote:
> On Fri, May 01, 2009 at 12:11:12PM -0400, Jeff Mahoney wrote:
>>  2.6.30-rc3 introduced some sanity checks in the VFS code to avoid NFS
>>  bugs by ensuring that lookup_one_len is always called under i_mutex.
>>
>>  This patch expands the i_mutex locking to enclose lookup_one_len. This was
>>  always required, but not not enforced in the reiserfs code since it
>>  does locking around the xattr interactions with the xattr_sem.
>>
>>  This is obvious enough, but it survived an overnight 50 thread ACL test.
> 
> It's not enough, unfortunately ;-/  It deals with the warning, but it
> leaves an actual hole in there.
> 
> Look: what happens if we mount it r/o without that directory and then
> remount r/w?  We get dentry for privroot, hash it (negative at that point),
> then do actual mkdir, unlock root and modify the ->d_compare() of root
> to reject lookups on that sucker.  Too late - in the meanwhile lookups
> might very well come and find privroot in dcache.
> 
> BTW, the way ->d_compare() is done in there is rather dumb -
> 	if (q1 == &priv_root->d_name)
> 		return -ENOENT;
> 	...
> would do just as well.  Why don't we do that lookup *once* (on ->get_sb(),
> before anything can come and race with us), and then just keep negative
> dentry if the directory hadn't been around?  And set d_compare() for root
> immediately after that lookup...
> 
> I've applied your patch as-is, and unless you have objections to the
> variant above I'll do that as incremental.  Comments?

Of course you're right about the lookup hole.

The lookup during remount is from when the code didn't enable xattrs
unconditionally for security and trusted attributes. The privroot would
only be created when mounted read-write with -oacl and/or -ouser_xattr.
Now xattrs are enabled uncondtionally, so any read-write mount will
create that if xattrs are enabled in the kernel. It used to avoid
caching a negative lookup that would never get used.

I don't have any objections to any of your suggestions.

Thanks!

- -Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkn+dp0ACgkQLPWxlyuTD7KuwQCgmrYQhuDy+HylXPL46Yb2R9Y+
Fr0AoIKRyL4mX1wCR+R9WN74zULTrbXT
=LSDi
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/