linux-kernel - Re: [UNIONFS] 00/29 Unionfs and related patches pre-merge review (v2)

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20080117060017.GC27894@ZenIV.linux.org.uk>
Date:	Thu, 17 Jan 2008 06:00:17 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Erez Zadok <ezk@...sunysb.edu>
Cc:	torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
	hch@...radead.org, viro@....linux.org.uk,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [UNIONFS] 00/29 Unionfs and related patches pre-merge review (v2)

After grep for locking-related things:

	* lock_parent(): who said that you won't get dentry moved
before managing to grab i_mutex on parent?  While we are at it,
who said that you won't get dentry moved between fetching d_parent
and doing dget()?  In that case parent could've been _freed_ before
you get to dget().

	* in create_parents():
+                       struct inode *inode = lower_dentry->d_inode;
+                       /*
+                        * If we get here, it means that we created a new
+                        * dentry+inode, but copying permissions failed.
+                        * Therefore, we should delete this inode and dput
+                        * the dentry so as not to leave cruft behind.
+                        */
+                       if (lower_dentry->d_op && lower_dentry->d_op->d_iput)
+                               lower_dentry->d_op->d_iput(lower_dentry,
+                                                          inode);
+                       else
+                               iput(inode);
+                       lower_dentry->d_inode = NULL;
+                       dput(lower_dentry);
+                       lower_dentry = ERR_PTR(err);
+                       goto out;
Really?  So what happens if it had become positive after your test and
somebody had looked it up in lower layer and just now happens to be
in the middle of operations on it?  Will be thucking frilled by that...

	* __unionfs_rename():
+       lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
+       err = vfs_rename(lower_old_dir_dentry->d_inode, lower_old_dentry,
+                        lower_new_dir_dentry->d_inode, lower_new_dentry);
+       unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry);

Uh-huh...  To start with, what guarantees that your lower_old_dentry
is still a child of your lower_old_dir_dentry?  What's more, you are
not checking the result of lock_rename(), i.e. asking for serious trouble.

	* revalidation stuff: err...  how the devil can it work for
directories, when there's nothing to prevent changes in underlying
layers between ->d_revalidate() and operation itself?  For the upper
layer (unionfs itself) everything's more or less fine, but the rest
of that...
--
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/