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 <[email protected]> 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/[email protected]?part=10
