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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] configfs: slab-use-after-free on target_item in
  configfs_symlink() [1]
--

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

configfs: fix use-after-free of default group in configfs_symlink()

This commit fixes a use-after-free of an embedded default group in
configfs_symlink() by dropping the parent_item reference before unlocking
dir and re-acquiring it after checking IS_DEADDIR(dir).

> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 89178f5d371a6..1b2c9048a920b 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c

[ ... ]

> @@ -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;
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can target_item suffer
from a similar use-after-free or NULL pointer dereference [1] if a
concurrent rmdir removes the target or an ancestor group after get_target()
returns?

CPU0                                CPU1
configfs_symlink()
  inode_unlock(dir)
  get_target()
    config_item_get(sd->s_element)
    configfs_get(sd)
                                    configfs_rmdir()
                                      ...
                                      unlink_group()
                                        unlink_obj()
                                          item->ci_group = NULL
                                          item->ci_parent = NULL
                                          config_item_put(item)
                                          config_group_put(group)
                                            // frees containing object
  inode_lock(dir)
  allow_link(..., target_item)  <-- use-after-free or NULL ci_parent
  config_item_put(target_item)  <-- use-after-free

get_target() grabs references to target_item and target_sd and drops the
path without holding any lock on the target directory or incrementing
target_sd->s_links (which happens later in create_link() under
configfs_symlink_mutex).  Because s_links is still 0, a concurrent
configfs_rmdir() can detach the target.  If target_item is a default group
embedded in a parent object without its own release callback, unlink_obj()
drops the parent group reference and frees the containing object while
target_item is still held; even for standalone items, unlink_obj() clears
ci_parent and ci_group before allow_link() runs.

[1] https://sashiko.dev/#/bug/linux-df6ae898-b1a6-4091-be3b-fbe4c177763f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-configfs-symlink-uaf-v1-1-1446154abdda@gmail.com?part=1