Re: [PATCH] gpio: sim: snapshot configfs leaf items before depending on them

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] gpio: sim: race with mkdir/rmdir in gpio_sim_device_lockup_configfs()
- [Critical] gpio: sim: extra leaf ref in gpio_sim_device_lockup_configfs() UAFs parent
- [High] gpio: sim: unchecked kcalloc() failure in gpio_sim_device_lockup_configfs()
--

commit 2fe24a96c3fb83559b6e4860a61df83ca81d16bb
Author: Yu Zhang <yuz08559@gmail.com>

gpio: sim: snapshot configfs leaf items before depending on them

This commit takes a reference-counted snapshot of leaf configfs items in
gpio_sim_device_lockup_configfs() while holding dev->lock, then calls
configfs_depend_item_unlocked() or configfs_undepend_item_unlocked() after
dropping the lock to avoid lock-order inversion with configfs rmdir.

> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index 898638e2794cb..e4d1318b3019c 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c
> @@ -978,27 +978,61 @@ static void
>  gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev, bool lock)
>  {
>  	struct configfs_subsystem *subsys = dev->group.cg_subsys;
> +	struct config_item **items = NULL, *item;
>  	struct gpio_sim_bank *bank;
>  	struct gpio_sim_line *line;
> -	struct config_item *item;
> +	size_t i, n;
>  
>  	/*
>  	 * The device only needs to depend on leaf entries. This is
>  	 * sufficient to lock up all the configfs entries that the
>  	 * instantiated, alive device depends on.
> +	 *
> +	 * The bank and line lists are only stable under dev->lock, but
> +	 * configfs_[un]depend_item_unlocked() must not be called under it:
> +	 * they take configfs-internal locks and that would invert the lock
> +	 * order against the configfs rmdir path, which removes lines and
> +	 * banks from these lists and frees them under dev->lock. So take a
> +	 * pinned snapshot of the leaf items under the lock - skipping any
> +	 * that are already being torn down - and then depend/undepend the
> +	 * snapshot with the lock dropped.
>  	 */
> -	list_for_each_entry(bank, &dev->bank_list, siblings) {
> -		list_for_each_entry(line, &bank->line_list, siblings) {
> -			item = line->hog ? &line->hog->item
> -					 : &line->group.cg_item;
> -
> -			if (lock)
> -				WARN_ON(configfs_depend_item_unlocked(subsys,
> -								      item));
> -			else
> -				configfs_undepend_item_unlocked(item);
> +	scoped_guard(mutex, &dev->lock) {
> +		n = 0;
> +		list_for_each_entry(bank, &dev->bank_list, siblings)
> +			list_for_each_entry(line, &bank->line_list, siblings)
> +				n++;
> +
> +		if (!n)
> +			return;
> +
> +		items = kcalloc(n, sizeof(*items), GFP_KERNEL);
> +		if (!items)
> +			return;

[Severity: High]
What happens if kcalloc() fails here when called from
gpio_sim_device_config_live_store()?

drivers/gpio/gpio-sim.c:gpio_sim_device_config_live_store() {
    ...
	if (live)
		gpio_sim_device_lockup_configfs(dev, true);

	scoped_guard(mutex, &dev->lock) {
		if (live == gpio_sim_device_is_live(dev))
			ret = -EPERM;
		else if (live)
			ret = gpio_sim_device_activate(dev);
		else
			gpio_sim_device_deactivate(dev);
	}

	if (live == !!ret)
		gpio_sim_device_lockup_configfs(dev, false);
}

Because gpio_sim_device_lockup_configfs() returns void, an allocation failure
on the lock == true path is silently ignored without depending on any leaf
items. gpio_sim_device_config_live_store() still proceeds, and when
gpio_sim_device_lockup_configfs(dev, false) later runs (either immediately on
activation failure / -EPERM or later on deactivation) and its kcalloc()
succeeds, configfs_undepend_item_unlocked() -> configfs_undepend_item() hits:

fs/configfs/dir.c:configfs_undepend_item() {
    ...
	sd = target->ci_dentry->d_fsdata;
	BUG_ON(sd->s_dependent_count < 1);

	sd->s_dependent_count -= 1;
    ...
}

Conversely, if kcalloc() fails on the lock == false path during deactivation,
gpio_sim_device_deactivate() has already torn down the device, so the
s_dependent_count references on all leaf items are permanently leaked and the
configfs directories can never be removed.

Should gpio_sim_device_lockup_configfs() propagate allocation errors, or can
the items be tracked without a dynamic allocation on the undepend path?

> +
> +		i = 0;
> +		list_for_each_entry(bank, &dev->bank_list, siblings) {
> +			list_for_each_entry(line, &bank->line_list, siblings) {
> +				item = line->hog ? &line->hog->item
> +						 : &line->group.cg_item;
> +				item = config_item_get_unless_zero(item);
> +				if (item)
> +					items[i++] = item;
> +			}
>  		}
> +		n = i;
>  	}

[Severity: Critical]
Can this snapshot logic still race with concurrent configfs mkdir and rmdir
operations in three ways?

First, when a hog directory is removed via configfs_rmdir(), the final
config_item_put() drops hog->item.ci_kref to 0 and calls
gpio_sim_hog_config_item_release(), which waits on dev->lock before clearing
line->hog:

drivers/gpio/gpio-sim.c:gpio_sim_hog_config_item_release() {
    ...
	scoped_guard(mutex, &dev->lock)
		line->hog = NULL;
    ...
}

If gpio_sim_device_lockup_configfs(dev, true) holds dev->lock at that point,
it sees line->hog != NULL, calls config_item_get_unless_zero(&line->hog->item)
which returns NULL, and skips the line completely without falling back to
&line->group.cg_item. Once dev->lock is dropped, line->hog becomes NULL, so a
subsequent gpio_sim_device_lockup_configfs(dev, false) snapshots
&line->group.cg_item and triggers BUG_ON(sd->s_dependent_count < 1) in
configfs_undepend_item().

Second, in configfs_rmdir(), unlink_obj() clears item->ci_group = NULL before
the final config_item_put() drops ci_kref to 0 and runs the release callback:

fs/configfs/dir.c:unlink_obj() {
    ...
	item->ci_group = NULL;
	item->ci_parent = NULL;

	/* Drop the reference for ci_entry */
	config_item_put(item);
}

An item can therefore be snapshotted via config_item_get_unless_zero() after
unlink_obj() has already cleared ci_group (or a concurrent configfs_rmdir()
can run unlink_obj() after dev->lock is dropped but before
configfs_depend_item_unlocked() runs). When configfs_depend_item_unlocked()
then inspects the snapshotted item, target->ci_group is NULL and
parent->cg_subsys dereferences a NULL pointer:

fs/configfs/dir.c:configfs_depend_item_unlocked() {
    ...
	parent = target->ci_group;
	if (configfs_is_root(&parent->cg_item)) {
		target_subsys = to_configfs_subsystem(to_config_group(target));
		root = parent;
	} else {
		target_subsys = parent->cg_subsys;
    ...
}

Third, in configfs_mkdir(), gpio_sim_bank_config_make_line_group() and
gpio_sim_line_config_make_hog_item() add the new item to bank->line_list or
line->hog with ci_kref == 1 under dev->lock before link_obj() sets
item->ci_group and before configfs_dir_set_ready() clears
CONFIGFS_USET_CREATING. Snapshotting the item in that window either
dereferences NULL target->ci_group in configfs_depend_item_unlocked() or fails
with -ENOENT in configfs_depend_prep() (leaving s_dependent_count == 0 and
later hitting BUG_ON(sd->s_dependent_count < 1) on undepend).

> +
> +	for (i = 0; i < n; i++) {
> +		if (lock)
> +			WARN_ON(configfs_depend_item_unlocked(subsys,
> +							      items[i]));
> +		else
> +			configfs_undepend_item_unlocked(items[i]);
> +		config_item_put(items[i]);
> +	}

[Severity: Critical]
Does holding an extra ci_kref reference on items[i] across the unlocked window
allow a child's release callback to run after its parent has already been
freed?

In configfs, unlink_obj() drops the child's reference on its parent
config_group during configfs_rmdir(child) via config_group_put(group), rather
than when the child's ci_kref reaches 0.

If a concurrent rmdir removes the leaf item (for example, right after
configfs_undepend_item_unlocked(items[i]) on deactivation, or before
configfs_depend_item_unlocked() on activation) while items[i] still holds a
reference from config_item_get_unless_zero(), configfs_rmdir(child) finishes
and drops the reference on the parent directory while child->ci_kref remains
1. Userspace can then immediately rmdir the now-empty parent line or bank
directory, freeing struct gpio_sim_line or struct gpio_sim_bank.

When config_item_put(items[i]) then drops the last reference on the child,
gpio_sim_hog_config_item_release() or gpio_sim_line_config_group_release()
dereferences the uncounted hog->parent or line->parent pointer into the freed
parent structure:

drivers/gpio/gpio-sim.c:gpio_sim_hog_config_item_release() {
	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
	struct gpio_sim_line *line = hog->parent;
	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);

	scoped_guard(mutex, &dev->lock)
		line->hog = NULL;
    ...
}

drivers/gpio/gpio-sim.c:gpio_sim_line_config_group_release() {
	struct gpio_sim_line *line = to_gpio_sim_line(item);
	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);

	scoped_guard(mutex, &dev->lock)
		list_del(&line->siblings);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026060049.60599-1-yuz08559@gmail.com?part=1