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

[sashiko-bot@kernel.org]

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