[PATCH] configfs: fix UAF race between rmdir and symlink

[Marcelo Mendes Spessoto Junior]

To avoid touching a target that is concurrently being removed,
configfs_get_config_item() and create_link() both rely on a hashed
dentry as proof that the config_item/configfs_dirent behind it is
still alive (using locks could lead to deadlocks, as described on
configfs_symlink).

However, configfs_remove_dir drops the last reference without
enforcing proper unhash over the dentry. This enables a possible
race condition where a symlink leads to UAF over a dentry that
has no reference but is still stale on the hash. Therefore, the
dentry removal must also enforce proper unhash, avoiding this
specific UAF scenario.

Verified against syzbot's C reproducer: no longer triggers the
WARN_ON/KASAN panics after this change.

Reported-by: syzbot+608f7f2a86361e18ba0b@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=608f7f2a86361e18ba0b
Signed-off-by: Marcelo Mendes Spessoto Junior <marcelomspessoto@xxxxxxxxx>
---
 fs/configfs/dir.c     | 15 +++++++++++++++
 fs/configfs/symlink.c | 30 ++++++++++++++++++++++++++----
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 3c88f13f1ca2..ec6f3550178a 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -411,6 +411,21 @@ static void configfs_remove_dir(struct dentry *d)
 {
 	struct dentry * parent = dget(d->d_parent);
 
+	/*
+	 * Unhash before dropping any reference to the dirent/item this
+	 * dentry pins: a concurrent configfs_get_config_item() (e.g. from
+	 * configfs_symlink()'s target resolution, which runs unlocked
+	 * against directories it doesn't otherwise own) only checks
+	 * d_unhashed() under d_lock before pinning the item.  Unhashing
+	 * first ensures that check reliably fails once we're past this
+	 * point, instead of racing the dirent/item's refcount reaching
+	 * zero while the dentry is still (briefly) hashed.
+	 */
+	spin_lock(&d->d_lock);
+	if (simple_positive(d))
+		__d_drop(d);
+	spin_unlock(&d->d_lock);
+
 	configfs_remove_dirent(d);
 
 	if (d_really_is_positive(d)) {
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 31eb28b27309..b8043a6b0b42 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -78,18 +78,40 @@ static int create_link(struct config_item *parent_item,
 		       struct config_item *item,
 		       struct dentry *dentry)
 {
-	struct configfs_dirent *target_sd = item->ci_dentry->d_fsdata;
+	struct dentry *target_dentry = item->ci_dentry;
+	struct configfs_dirent *target_sd;
 	char *body;
 	int ret;
 
-	if (!configfs_dirent_is_ready(target_sd))
+	/*
+	 * item is pinned by the caller, but that only keeps the config_item
+	 * itself alive.  item->ci_dentry's configfs_dirent (and thus its
+	 * s_count) is a separate refcount that a concurrent rmdir of this
+	 * same directory can drop to zero and free independently -- see the
+	 * matching d_lock/d_unhashed() dance in configfs_get_config_item()
+	 * and the unhash-before-free ordering configfs_remove_dir() now
+	 * guarantees.  Do the same check here instead of trusting
+	 * target_dentry->d_fsdata unconditionally.
+	 */
+	spin_lock(&target_dentry->d_lock);
+	if (d_unhashed(target_dentry)) {
+		spin_unlock(&target_dentry->d_lock);
 		return -ENOENT;
+	}
+	target_sd = configfs_get(target_dentry->d_fsdata);
+	spin_unlock(&target_dentry->d_lock);
+
+	if (!configfs_dirent_is_ready(target_sd)) {
+		configfs_put(target_sd);
+		return -ENOENT;
+	}
 
 	body = kzalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!body)
+	if (!body) {
+		configfs_put(target_sd);
 		return -ENOMEM;
+	}
 
-	configfs_get(target_sd);
 	spin_lock(&configfs_dirent_lock);
 	if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
 		spin_unlock(&configfs_dirent_lock);
-- 
2.55.0