[PATCH v2 0/2] configfs: fix use-after-free races in configfs_symlink()

[PATCH v2 0/2] configfs: fix use-after-free races in configfs_symlink()

[Izaac Chong]

configfs_symlink() drops the parent directory's lock while it resolves
the link target. Two races in that window are fixed here:

1/2 fixes a use-after-free when the parent is a default group whose
    containing object is freed by a concurrent rmdir (syzbot,
    nvmet_port_subsys_allow_link).
2/2 pins the target against rmdir before ->allow_link(), so drivers
    that walk the target's ->ci_parent do not see it unlinked
    (NULL deref in uvcg_streaming_header_allow_link). Found by Sashiko
    review of v1.

Changes in v2:
- Add patch 2/2. Patch 1/2 is unchanged.

Testing (QEMU + KASAN, v7.4-rc2): the syzbot
reproducer crashes without 1/2; a UVC symlink-vs-rmdir reproducer
crashes without 2/2 (with 1/2 applied). With both patches, both
reproducers plus an nvmet target-side race run clean, normal
link/unlink/rmdir still work, and rmdir of a linked target still
returns -EBUSY. syzbot #syz test is currently disabled.

Not tested: lockdep was off; LIO (target_fabric_mappedlun_link) was not
exercised.

---
Izaac Chong (2):
      configfs: fix use-after-free of default group in configfs_symlink()
      configfs: pin symlink target against rmdir before ->allow_link()

 fs/configfs/symlink.c | 66 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 20 deletions(-)
---
base-commit: 97d73c8251226cf27cc6656b4c542869bb8c69fa
change-id: 20261114-configfs-symlink-uaf-c5d84bb6a976

Best regards,
--  
Izaac Chong <izaacchong934@gmail.com>

[PATCH v2 1/2] configfs: fix use-after-free of default group in configfs_symlink()

[Izaac Chong]

configfs_symlink() drops the parent directory's inode lock while it
resolves the link target, and keeps a reference to parent_item across
that window. When the parent is a default group, such as
ports/<n>/subsystems/ in the NVMe target, parent_item is embedded in
the containing object (struct nvmet_port). A reference on the embedded
item does not keep the containing object alive, so a concurrent rmdir of
the port can free it. allow_link() and the final config_item_put() then
touch freed memory:

  BUG: KASAN: slab-use-after-free in nvmet_port_subsys_allow_link+0x41/0x2e0
  Call Trace:
   nvmet_port_subsys_allow_link+0x41/0x2e0 drivers/nvme/target/configfs.c:1059
   configfs_symlink+0x59a/0x1030 fs/configfs/symlink.c:196
   vfs_symlink+0x18b/0x330 fs/namei.c:5794

detach_groups() marks a default group's directory S_DEAD while holding
that directory's inode lock, before the containing object can be freed.
So drop the parent_item reference before unlocking dir, and after
re-taking the lock return -ENOENT if dir is dead. Otherwise look
parent_item up again, which is safe while dir is locked and alive.

Fixes: 351e5d869e5a ("configfs: fix a deadlock in configfs_symlink()")
Reported-by: syzbot+b0996ac2197dd7420c3e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b0996ac2197dd7420c3e
Cc: linux-nvme@lists.infradead.org
Signed-off-by: Izaac Chong <izaacchong934@gmail.com>
---
 fs/configfs/symlink.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 89178f5d371a..1b2c9048a920 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -166,11 +166,11 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 
 	parent_item = configfs_get_config_item(dentry->d_parent);
 	type = parent_item->ci_type;
+	config_item_put(parent_item);
 
-	ret = -EPERM;
 	if (!type || !type->ct_item_ops ||
 	    !type->ct_item_ops->allow_link)
-		goto out_put;
+		return -EPERM;
 
 	/*
 	 * This is really sick.  What they wanted was a hybrid of
@@ -200,7 +200,20 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 	ret = get_target(symname, &target_item, &target_sd, dentry->d_sb);
 	inode_lock(dir);
 	if (ret)
-		goto out_put;
+		return ret;
+
+	/*
+	 * parent_item may be a default group embedded in an object that a
+	 * concurrent rmdir freed while dir was unlocked, so we dropped our
+	 * reference above.  detach_groups() marks dir S_DEAD under its inode
+	 * lock before the containing object can go away, so it is safe to
+	 * look parent_item up again only if dir is still alive.
+	 */
+	if (IS_DEADDIR(dir)) {
+		ret = -ENOENT;
+		goto out_put_target;
+	}
+	parent_item = configfs_get_config_item(dentry->d_parent);
 
 	if (dentry->d_inode || d_unhashed(dentry))
 		ret = -EEXIST;
@@ -218,11 +231,10 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 						     target_item);
 	}
 
+	config_item_put(parent_item);
+out_put_target:
 	configfs_put(target_sd);
 	config_item_put(target_item);
-
-out_put:
-	config_item_put(parent_item);
 	return ret;
 }
 

-- 
Git-143)

Re: [PATCH v2 1/2] configfs: fix use-after-free of default group in configfs_symlink()

[sashiko-bot]

> configfs_symlink() drops the parent directory's inode lock while it
> resolves the link target, and keeps a reference to parent_item across
> that window. When the parent is a default group, such as
> ports/<n>/subsystems/ in the NVMe target, parent_item is embedded in
> the containing object (struct nvmet_port). A reference on the embedded
> item does not keep the containing object alive, so a concurrent rmdir of
> the port can free it. allow_link() and the final config_item_put() then
> touch freed memory:
> 
>   BUG: KASAN: slab-use-after-free in nvmet_port_subsys_allow_link+0x41/0x2e0
>   Call Trace:
>    nvmet_port_subsys_allow_link+0x41/0x2e0 drivers/nvme/target/configfs.c:1059
>    configfs_symlink+0x59a/0x1030 fs/configfs/symlink.c:196
>    vfs_symlink+0x18b/0x330 fs/namei.c:5794
> 
> detach_groups() marks a default group's directory S_DEAD while holding
> that directory's inode lock, before the containing object can be freed.
> So drop the parent_item reference before unlocking dir, and after
> re-taking the lock return -ENOENT if dir is dead. Otherwise look
> parent_item up again, which is safe while dir is locked and alive.
> 
> Fixes: 351e5d869e5a ("configfs: fix a deadlock in configfs_symlink()")
> Reported-by: syzbot+b0996ac2197dd7420c3e@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=b0996ac2197dd7420c3e
> Cc: linux-nvme@lists.infradead.org
> Signed-off-by: Izaac Chong <izaacchong934@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-configfs-symlink-uaf-v2-0-48c9cf068a91@gmail.com?part=1

[PATCH v2 2/2] configfs: pin symlink target against rmdir before ->allow_link()

[Izaac Chong]

configfs_symlink() looks up the link target with get_target(), which
takes references on the target item and dirent but does not stop the
target from being removed. The target is only pinned (target_sd->s_links
incremented, refusing CONFIGFS_USET_DROPPING) later, in create_link(),
after ->allow_link() has run.

A concurrent rmdir of the target can therefore complete in between.
unlink_obj() then clears target->ci_parent and ->ci_group, and
->allow_link() implementations that walk the target's parents
dereference NULL. With the USB gadget UVC function:

  BUG: KASAN: null-ptr-deref in uvcg_streaming_header_allow_link+0xc3/0x270
  Read of size 8 at addr 0000000000000030
  Call Trace:
   uvcg_streaming_header_allow_link
   configfs_symlink
   vfs_symlink
   filename_symlinkat
   __x64_sys_symlink

target_fabric_mappedlun_link() also walks the target's ->ci_parent.

Take the s_links pin before calling ->allow_link() instead, under
configfs_dirent_lock and only if the target is not being dropped, as
create_link() did. configfs_detach_prep() sets CONFIGFS_USET_DROPPING
before checking s_links under the same lock, so the target either fails
to pin (-ENOENT) or rmdir sees the pin and returns -EBUSY. On success the
link keeps the count and configfs_unlink() drops it as before; on failure
it is dropped again.

Fixes: 4768e9b18dc6 ("[PATCH] configfs: Fix symlink() to a removing item")
Link: https://sashiko.dev/#/bug/linux-df6ae898-b1a6-4091-be3b-fbe4c177763f
Signed-off-by: Izaac Chong <izaacchong934@gmail.com>
---
 fs/configfs/symlink.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 1b2c9048a920..52f0f3a6cd7e 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -90,23 +90,11 @@ static int create_link(struct config_item *parent_item,
 		return -ENOMEM;
 
 	configfs_get(target_sd);
-	spin_lock(&configfs_dirent_lock);
-	if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
-		spin_unlock(&configfs_dirent_lock);
-		configfs_put(target_sd);
-		kfree(body);
-		return -ENOENT;
-	}
-	target_sd->s_links++;
-	spin_unlock(&configfs_dirent_lock);
 	ret = configfs_get_target_path(parent_item, item, body);
 	if (!ret)
 		ret = configfs_create_link(target_sd, parent_item->ci_dentry,
 					   dentry, body);
 	if (ret) {
-		spin_lock(&configfs_dirent_lock);
-		target_sd->s_links--;
-		spin_unlock(&configfs_dirent_lock);
 		configfs_put(target_sd);
 		kfree(body);
 	}
@@ -220,8 +208,28 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 	else
 		ret = inode_permission(&nop_mnt_idmap, dir,
 				       MAY_WRITE | MAY_EXEC);
-	if (!ret)
-		ret = type->ct_item_ops->allow_link(parent_item, target_item);
+	if (ret)
+		goto out_put_parent;
+
+	/*
+	 * Pin the target against rmdir before calling ->allow_link().
+	 * configfs_detach_prep() refuses to remove a dirent with s_links
+	 * set, and sets CONFIGFS_USET_DROPPING (under configfs_dirent_lock)
+	 * before it checks s_links.  Without the pin, a concurrent rmdir can
+	 * unlink the target (clearing ->ci_parent and ->ci_group) while
+	 * ->allow_link() looks at it.  On success the link owns the count,
+	 * which configfs_unlink() drops.
+	 */
+	spin_lock(&configfs_dirent_lock);
+	if (target_sd->s_type & CONFIGFS_USET_DROPPING)
+		ret = -ENOENT;
+	else
+		target_sd->s_links++;
+	spin_unlock(&configfs_dirent_lock);
+	if (ret)
+		goto out_put_parent;
+
+	ret = type->ct_item_ops->allow_link(parent_item, target_item);
 	if (!ret) {
 		mutex_lock(&configfs_symlink_mutex);
 		ret = create_link(parent_item, target_item, target_sd, dentry);
@@ -230,7 +238,13 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 			type->ct_item_ops->drop_link(parent_item,
 						     target_item);
 	}
+	if (ret) {
+		spin_lock(&configfs_dirent_lock);
+		target_sd->s_links--;
+		spin_unlock(&configfs_dirent_lock);
+	}
 
+out_put_parent:
 	config_item_put(parent_item);
 out_put_target:
 	configfs_put(target_sd);

-- 
Git-143)

Re: [PATCH v2 2/2] configfs: pin symlink target against rmdir before ->allow_link()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] configfs: missing CONFIGFS_USET_CREATING check before
  allow_link()
- [Medium] configfs: target_sd->s_links modified outside
  configfs_symlink_mutex in configfs_symlink()
--

commit e7abed27dac194611cbdf483f151d7345ac8c114
Author: Izaac Chong <izaacchong934@gmail.com>

configfs: pin symlink target against rmdir before ->allow_link()

This commit moves the target_sd->s_links increment and the
CONFIGFS_USET_DROPPING check from create_link() to before ->allow_link() in
configfs_symlink() so a concurrent rmdir cannot unlink the target while
->allow_link() runs.

Link: https://sashiko.dev/#/bug/linux-df6ae898-b1a6-4091-be3b-fbe4c177763f

> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 1b2c9048a920b..52f0f3a6cd7e0 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
[ ... ]
> @@ -220,8 +208,28 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  	else
>  		ret = inode_permission(&nop_mnt_idmap, dir,
>  				       MAY_WRITE | MAY_EXEC);
> -	if (!ret)
> -		ret = type->ct_item_ops->allow_link(parent_item, target_item);
> +	if (ret)
> +		goto out_put_parent;
> +
> +	/*
> +	 * Pin the target against rmdir before calling ->allow_link().
> +	 * configfs_detach_prep() refuses to remove a dirent with s_links
> +	 * set, and sets CONFIGFS_USET_DROPPING (under configfs_dirent_lock)
> +	 * before it checks s_links.  Without the pin, a concurrent rmdir can
> +	 * unlink the target (clearing ->ci_parent and ->ci_group) while
> +	 * ->allow_link() looks at it.  On success the link owns the count,
> +	 * which configfs_unlink() drops.
> +	 */
> +	spin_lock(&configfs_dirent_lock);
> +	if (target_sd->s_type & CONFIGFS_USET_DROPPING)
> +		ret = -ENOENT;
> +	else
> +		target_sd->s_links++;
> +	spin_unlock(&configfs_dirent_lock);
> +	if (ret)
> +		goto out_put_parent;
> +
> +	ret = type->ct_item_ops->allow_link(parent_item, target_item);

[Severity: Medium]
Does checking only CONFIGFS_USET_DROPPING here still allow ->allow_link() to
run against a target that has CONFIGFS_USET_CREATING set?

configfs_create_dir() hashes the target dentry with d_make_persistent()
before populate_attrs() and populate_groups() finish, while
!configfs_dirent_is_ready(target_sd) is only checked later inside
create_link().  If the concurrent configfs_mkdir() fails during attachment,
its out_unlink path tears the target down without calling
configfs_detach_prep(), setting CONFIGFS_USET_DROPPING or checking s_links:

CPU0                              CPU1
configfs_mkdir()
  configfs_attach_group()
    configfs_create_dir()
      d_make_persistent()
                                  configfs_symlink()
                                    get_target()
                                    target_sd->s_links++
    populate_groups() -> fails
  unlink_group(group)
    unlink_obj(item)
      item->ci_parent = NULL
                                    ->allow_link(parent_item, target_item)
                                      target_item->ci_parent
                                        <-- NULL pointer dereference

Checking !configfs_dirent_is_ready(target_sd) under configfs_dirent_lock
alongside CONFIGFS_USET_DROPPING before incrementing s_links would prevent
pinning an item before configfs_dir_set_ready() has cleared
CONFIGFS_USET_CREATING.

>  	if (!ret) {
>  		mutex_lock(&configfs_symlink_mutex);
>  		ret = create_link(parent_item, target_item, target_sd, dentry);
> @@ -230,7 +238,13 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  			type->ct_item_ops->drop_link(parent_item,
>  						     target_item);
>  	}
> +	if (ret) {
> +		spin_lock(&configfs_dirent_lock);
> +		target_sd->s_links--;
> +		spin_unlock(&configfs_dirent_lock);
> +	}

[Severity: Medium]
Can modifying target_sd->s_links outside configfs_symlink_mutex cause a
concurrent configfs_rmdir() on the target to fail spuriously with -EBUSY when
configfs_symlink() fails?

Commit 9a73d78cda75 ("[PATCH] configfs: Fix failing symlink() making rmdir()
fail") serialized s_links attachment and rollback under
configfs_symlink_mutex so configfs_rmdir() cannot observe a transient
s_links increment from a failing symlink():

CPU0                              CPU1
configfs_symlink()
  target_sd->s_links++
  ->allow_link() -> fails
                                  configfs_rmdir()
                                    mutex_lock(&configfs_symlink_mutex)
                                    configfs_detach_prep()
                                      sees target_sd->s_links > 0
                                      returns -EBUSY
                                    mutex_unlock(&configfs_symlink_mutex)
  target_sd->s_links--

The same race occurs if create_link() fails after releasing
configfs_symlink_mutex and before target_sd->s_links is decremented.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-configfs-symlink-uaf-v2-0-48c9cf068a91@gmail.com?part=2