Re: [PATCH v13 10/14] pmdomain: rockchip: cycle optional power-domain resets on power-on

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pmdomain: rockchip: UAF of reset control on probe error path
--

commit 7f227aa8bc180bf2cac448c39f64d020d6dc07cf
Author: Jiaxing Hu <gahing@gahingwoo.com>

pmdomain: rockchip: cycle optional power-domain resets on power-on

Some Rockchip domains come out of power-on with their bus interface in an
undefined state. On the RK3576 NPU this shows up as a hang on the first
register access after the domain is switched on, and pulsing the domain's
resets at this point clears it.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index 39988efd86aaa..6cc8d6efda905 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c

[ ... ]

> @@ -688,6 +690,21 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>  		if (ret < 0)
>  			goto out;
>  
> +		/*
> +		 * Optional: some domains need their resets cycled once power
> +		 * is on. This goes BEFORE the settle delay, not after: a
> +		 * domain that asks for both is asking to settle before the
> +		 * QoS registers answer, and a reset deasserted after the
> +		 * delay would leave nothing between it and the QoS writes.
> +		 * On RK3576 the reset being cycled is the NPU core's bus
> +		 * interface, which is what those writes go through.
> +		 */
> +		if (pd->resets) {
> +			reset_control_assert(pd->resets);

[Severity: High]
Can a concurrent call to power the domain trigger a use-after-free here?

If rockchip_pm_domain_probe() fails, the error path calls
rockchip_pm_domain_cleanup(), which invokes rockchip_pm_remove_one_domain().
For domains with attached subdomains, pm_genpd_remove() returns -EBUSY and
the driver ignores this failure, proceeding to free resources.

If pd->resets is freed in rockchip_pm_remove_one_domain() but the domain
remains registered, could a concurrent request to power the domain (via another
device probe or sysfs) invoke rockchip_pd_power(), evaluate the dangling
pd->resets pointer as true, and dereference it?

> +			usleep_range(10, 20);
> +			reset_control_deassert(pd->resets);
> +		}
> +
>  		/* Some domains need to settle before the QoS registers answer. */
>  		if (pd->info->delay_us)
>  			udelay(pd->info->delay_us);

[ ... ]

> @@ -949,6 +975,7 @@ static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
>  
>  	clk_bulk_unprepare(pd->num_clks, pd->clks);
>  	clk_bulk_put(pd->num_clks, pd->clks);
> +	reset_control_put(pd->resets);

[Severity: High]
Should pd->resets be set to NULL here?

The reset control is freed here, but the pd->resets pointer is left dangling.
This is unlike pd->num_clks below, which is safely cleared.

>  
>  	/* protect the zeroing of pm->num_clks */
>  	mutex_lock(&pd->pmu->mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915104328.45901-1-gahing@gahingwoo.com?part=10