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

[Yixun Lan <dlan@kernel.org>]

Hi ALL,


On 03:55 Thu 08 Oct     , sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] pmdomain: missing .remove callback causes UAF on driver unbind
> - [High] pmdomain: probe fails unconditionally if ALWAYS_ON domain is powered off
> --
> 
> commit ce2204cb1ba682f1391fc6691c412bb036ad43ca
> Author: Yixun Lan <dlan@kernel.org>
> Date:   Thu Oct 8 03:44:30 2026 +0000
> 
> pmdomain: spacemit: Add power domain driver
> 
> This commit introduces a Generic PM Domain driver for SpacemiT SoCs. It
> provides support for managing power domains across different application
> scenarios through software or hardware control modes.
> 
> > diff --git a/drivers/pmdomain/spacemit/spacemit-pm-domains.c b/drivers/pmdomain/spacemit/spacemit-pm-domains.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/drivers/pmdomain/spacemit/spacemit-pm-domains.c
> 
> [ ... ]
> 
> > +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;
> > +	int ret;
> > +
> > +	pd = devm_kzalloc(pmu->dev, sizeof(*pd), GFP_KERNEL);
> > +	if (!pd)
> > +		return -ENOMEM;
> > +
> > +	pd->pm_index        = id;
> > +	pd->param           = param;
> > +	pd->gpmu            = pmu;
> > +	pd->genpd.name      = param->name;
> > +	pd->genpd.flags     = param->flags;
> > +	pd->genpd.power_off = spacemit_pd_power_off;
> > +	pd->genpd.power_on  = spacemit_pd_power_on;
> > +
> > +	ret = pm_genpd_init(&pd->genpd, NULL, !spacemit_pm_get_state(pmu, pd));
> 
> [Severity: High]
> Will this unconditionally fail probe if the bootloader leaves the audio
> domain powered off?
> 
I've checked, the bootloader always set audio domain on before linux starts

> The k3_domain_params define the audio domain with GENPD_FLAG_ALWAYS_ON, but
> here pm_genpd_init() is called with the inverse of the current hardware
> state (is_off = !spacemit_pm_get_state()).
> 
this is still correct, but the name of function makes people a bit confused,
 I could change it to a more obvious one to match pm_genpd_init(.., is_off),
probably spacemit_pm_is_off_state()

> If the hardware happens to be off, is_off evaluates to true. However, the
> genpd core explicitly rejects initializing always-on domains in the off
> state:
> 
> drivers/pmdomain/core.c:pm_genpd_init() {
>     ...
>     /* Always-on domains must be powered on at initialization. */
>     if ((genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd)) &&
>             !genpd_status_on(genpd)) {
>         pr_err("always-on PM domain %s is not on\n", genpd->name);
>         return -EINVAL;
>     }
>     ...
> }
> 
> Since a failure here aborts spacemit_pm_domain_probe(), does the driver
> need to explicitly power on always-on domains before passing them to the
> genpd framework?
> 
The case said here won't happen, and the system will be in unusable state
if audio domain in a 'off' state, so leave kernel yell and abort, that's
expected.

> > +	if (ret)
> > +		return ret;
> > +
> > +	pmu->domains[id] = pd;
> > +
> > +	return 0;
> > +}
> 
> [ ... ]
> 
> > +static int spacemit_pm_domain_probe(struct auxiliary_device *adev,
> > +				    const struct auxiliary_device_id *id)
> > +{
> > +	struct spacemit_ccu_adev *rdev = to_spacemit_ccu_adev(adev);
> > +	const struct spacemit_pm_aux_data *data;
> > +	struct device *dev = &adev->dev;
> > +	struct spacemit_pmu *pmu;
> > +	int err, i;
> > +
> > +	data = (const struct spacemit_pm_aux_data *)id->driver_data;
> > +
> > +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> 
> [Severity: High]
> Could this cause a use-after-free on module unload or driver unbind?
> 
> The driver dynamically allocates the pmu structure and domain entries using
> devres APIs like devm_kzalloc() and devm_kcalloc(). During probe, these
> domains and the provider are registered with the genpd core using
> pm_genpd_init() and of_genpd_add_provider_onecell().
> 
> However, the spacemit_pm_domain_driver below lacks a .remove callback. When
> the driver is unbound, devres will automatically free the allocated pmu and
> domain memory, but pm_genpd_remove() and of_genpd_del_provider() are never
> called.
> 
> Does this leave dangling pointers in the core gpd_list and
> of_genpd_providers lists that could panic the kernel when accessed?
> 
That's something I will fix in next version, I've changed the driver to
support be built as a module.. while not implement a complete remove() function

> > +	if (!pmu)
> > +		return -ENOMEM;
> 
> [ ... ]
> 
> > +static struct auxiliary_driver spacemit_pm_domain_driver = {
> > +	.probe          = spacemit_pm_domain_probe,
> > +	.id_table       = spacemit_pm_domain_ids,
> > +};
> > +module_auxiliary_driver(spacemit_pm_domain_driver);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261008-04-k3-pm-support-v2-0-6f778a53dd2d@kernel.org?part=2

-- 
Yixun Lan (dlan)