configfs_lookup(): don't leave ->s_dentry dangling on failure

Normally ->s_dentry is cleared when dentry it's pointing to becomes
negative (on eviction, realistically).  However, that only happens
if dentry gets to be positive in the first place; in case of inode
allocation failure dentry never becomes positive, so ->d_iput()
is not called at all.

We do part of what normally would've been done by configfs_d_iput()
(dropping the reference to configfs_dirent) manually, but we do
not clear ->s_dentry there.  Sloppy as it is, it does not matter in
case of configfs_create_{dir,link}() - there configfs_dirent does
not survive dropping the sole reference to it.

However, for configfs_lookup() it *does* survive, with a dangling
pointer to soon to be freed dentry sitting it its ->s_dentry.

Subsequent getdents(2) in that directory will end up dereferencing
that pointer in order to pick the inode number.  Use after free...

This is the minimal fix; the right approach is to set the linkage
between dentry and configfs_dirent only after we know that we have
an inode, but that takes more surgery and the bug had been there
since 2006, so...

Fixes: 3d0f89bb1694 ("configfs: Add permission and ownership to configfs objects") # 2.6.16-rc3
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 362b6ff9b908f1b216426feb995f4b4215b72795..e84483a0836d8e88bf94adde476414b05b1e1893 100644 (file)
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -486,6 +486,9 @@ static struct dentry * configfs_lookup(struct inode *dir,
 
                        inode = configfs_create(dentry, mode);
                        if (IS_ERR(inode)) {
+                               spin_lock(&configfs_dirent_lock);
+                               sd->s_dentry = NULL;
+                               spin_unlock(&configfs_dirent_lock);
                                configfs_put(sd);
                                return ERR_CAST(inode);
                        }