Re: [PATCH 2/3] pmdomain: spacemit: Add power domain driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] pmdomain: spacemit: missing .remove callback causes UAF on driver unbind
- [High] pmdomain: spacemit: unchecked pm_genpd_init() return leads to NULL deref panic
- [High] pmdomain: spacemit: global gpmu assignment races with consumer probe
- [Medium] pmdomain: spacemit: uninitialized stack variable used if regmap_read() fails
- [Low] pmdomain: spacemit: missing architecture dependency in Kconfig
--

commit af62cacb34c99353b794b2101862dcbbf7c19cfb
Author: Yixun Lan <dlan@kernel.org>

    pmdomain: spacemit: Add power domain driver
    
    SpacemiT's PMU (Power Management Unit) consist of several power domains
    which can be managed independently, depending on different application
    scenario, each domain can be powered on/off for saving power.

> diff --git a/drivers/pmdomain/spacemit/Kconfig b/drivers/pmdomain/spacemit/Kconfig
> new file mode 100644
> index 0000000000000..4b927beedc7c0
> --- /dev/null
> +++ b/drivers/pmdomain/spacemit/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +menu "SpacemiT PM Domains"
> +
> +config SPACEMIT_PM_DOMAINS
> +	bool "PM Domain driver for SpacemiT SoC"
> +	depends on PM
> +	depends on OF
> +	depends on MFD_SYSCON

[Severity: Low]
Should this also have a dependency on ARCH_SPACEMIT or COMPILE_TEST?

Without it, users configuring kernels for any architecture will be prompted 
to build this SoC-specific driver as long as the other dependencies are met.

[ ... ]

> diff --git a/drivers/pmdomain/spacemit/pm_domains.c b/drivers/pmdomain/spacemit/pm_domains.c
> new file mode 100644
> index 0000000000000..d563e4e4e232e
> --- /dev/null
> +++ b/drivers/pmdomain/spacemit/pm_domains.c
> @@ -0,0 +1,448 @@

[ ... ]

> +static int spacemit_pd_power_on(struct generic_pm_domain *domain)
> +{
> +	struct spacemit_pm_domain *spd = container_of(domain, struct spacemit_pm_domain, genpd);
> +	const struct spacemit_pm_domain_param *p = spd->param;
> +	unsigned int val;
> +	int ret = 0;
> +
> +	regmap_read(gpmu->regmap, APMU_POWER_STATUS_REG, &val);
> +
> +	if (!p->use_hw && (val & BIT(p->bit_pwr_stat))) {

[Severity: Medium]
What happens if regmap_read() fails and returns an error code?

If regmap_read() fails here, the output parameter val will remain completely 
uninitialized. The uninitialized stack variable will then be evaluated to
determine the current power state.

Would it be better to check the return value of regmap_read(), or at least
initialize val to zero?

[ ... ]

> +static bool spacemit_pm_get_state(struct spacemit_pmu *pmu,
> +				  struct spacemit_pm_domain *pd)
> +{
> +	const struct spacemit_pm_domain_param *p = pd->param;
> +	u32 reg, bit;
> +
> +	regmap_read(pmu->regmap, APMU_POWER_STATUS_REG, &reg);
> +
> +	bit = p->use_hw ? BIT(pd->param->bit_hw_pwr_stat) :
> +		BIT(pd->param->bit_pwr_stat);
> +
> +	return !!(reg & bit);
> +}

[Severity: Medium]
Similarly here, if regmap_read() fails, the uninitialized variable reg
is used to return the power state.

> +
> +static int spacemit_pm_add_one_domain(struct spacemit_pmu *pmu, int id,
> +				      const struct spacemit_pm_domain_param *param)
> +{
> +	struct spacemit_pm_domain *pd;
> +
> +	pd = devm_kzalloc(pmu->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +
> +	pd->pm_index        = id;
> +	pd->param           = param;
> +	pd->genpd.name      = param->name;
> +	pd->genpd.power_off = spacemit_pd_power_off;
> +	pd->genpd.power_on  = spacemit_pd_power_on;
> +
> +	pm_genpd_init(&pd->genpd, NULL, !spacemit_pm_get_state(pmu, pd));
> +
> +	pmu->domains[id] = pd;
> +
> +	return 0;
> +}

[Severity: High]
Does this code need to check the return value of pm_genpd_init()?

If pm_genpd_init() fails (for example, due to a memory allocation failure 
inside genpd_alloc_data()), the failure is ignored and the uninitialized 
domain is still stored in the domains array.

> +
> +static void spacemit_pm_domain_cleanup(struct spacemit_pmu *pmu)
> +{
> +	int i;
> +
> +	for (i = 0; i < pmu->num_domains; i++) {
> +		if (pmu->domains[i])
> +			pm_genpd_remove(&pmu->domains[i]->genpd);
> +	}
> +}

[Severity: High]
If a domain previously failed initialization in pm_genpd_init() but was still
added to the array, will this unconditionally call pm_genpd_remove() on it?

Because pm_genpd_remove() calls list_del(&genpd->gpd_list_node), running it
on a domain with a zero-filled uninitialized list node will cause an immediate
NULL pointer dereference kernel panic.

[ ... ]

> +	err = of_genpd_add_provider_onecell(dev->of_node, &pmu->genpd_data);
> +	if (err) {
> +		dev_err(dev, "failed to add provider: %d\n", err);
> +		goto err_out;
> +	}
> +
> +	gpmu = pmu;

[Severity: High]
Could a consumer device probe race with the global gpmu assignment?

The call to of_genpd_add_provider_onecell() registers the PM domains and makes
them visible to consumers. Once this returns, deferred consumer devices can 
immediately probe and attempt to power on the domain by calling 
spacemit_pd_power_on(), which dereferences gpmu->regmap.

Since gpmu = pmu happens after the provider registration, is there a window 
where a concurrent consumer probe could see gpmu as NULL and trigger a
NULL pointer dereference?

[ ... ]

> +static struct platform_driver spacemit_pm_domain_driver = {
> +	.probe = spacemit_pm_domain_probe,
> +	.driver = {
> +		.name           = "spacemit-pm-domain",
> +		.of_match_table = spacemit_pm_domain_dt_match,
> +	},
> +};

[Severity: High]
Is it safe to allow this driver to be unbound without a .remove callback or 
setting .suppress_bind_attrs = true?

If a root user manually unbinds the driver via sysfs:
/sys/bus/platform/drivers/spacemit-pm-domain/unbind

All devres managed memory (like the struct spacemit_pm_domain allocations) 
will be automatically freed. Because the driver lacks a .remove callback, it 
fails to unregister the PM domains from the generic PM domain framework 
(pm_genpd_remove) and the device tree provider list (of_genpd_del_provider). 

This leaves the framework with dangling pointers to the freed memory, causing a
use-after-free upon any subsequent access.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-04-k3-pm-support-v1-0-0acd2b36b96f@kernel.org?part=2