Re: [PATCH v3 2/5] pmdomain: mediatek: Fix possible nullptr KP in HWV cleanup/on-check

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Sleeping in atomic context in HW_VOTER power domain callbacks
- [High] Use-After-Free during probe error cleanup due to incorrect teardown order
- [High] Unbalanced clock and regulator disable during error cleanup for domains initialized with MTK_SCPD_KEEP_DEFAULT_OFF
--

--- Patch [2]: [PATCH v3 2/5] pmdomain: mediatek: Fix possible nullptr KP in HWV cleanup/on-check ---
commit 6f66af4cf25683016b57cc9dd8c70be44004a959
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: pmdomain: mediatek: Fix possible nullptr KP in HWV cleanup/on-check

This commit fixes a possible NULL pointer dereference in the cleanup and
on-check paths for HW_VOTER type power domains during probe failure. It checks
the MTCMOS type and calls the appropriate functions for checking status and
powering off.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index e1cfd42234734..f0a6339affd75 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -393,9 +393,8 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
>  	return ret;
>  };
>  
> -static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> +static int scpsys_hwv_power_off_internal(struct scpsys_domain *pd)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but can calling sleeping functions
here cause a kernel panic due to scheduling while atomic?

The driver sets GENPD_FLAG_IRQ_SAFE for HWV domains in scpsys_add_one_domain().
This causes the genpd framework to acquire a spinlock and disable interrupts
before calling scpsys_hwv_power_off() and scpsys_hwv_power_on().

However, these functions then call sleeping functions like
clk_bulk_prepare_enable() and scpsys_regulator_enable(), which would trigger
a scheduling while atomic BUG.

> -	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
>  	const struct scpsys_hwv_domain_data *hwv = pd->hwv_data;
>  	struct scpsys *scpsys = pd->scpsys;

[ ... ]

> @@ -973,6 +992,7 @@ static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *paren
>  
>  static void scpsys_remove_one_domain(struct scpsys_domain *pd)
>  {
> +	struct scpsys *scpsys = pd->scpsys;
>  	int ret;
>  
>  	/*
> @@ -984,8 +1004,14 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
>  		dev_err(pd->scpsys->dev,
>  			"failed to remove domain '%s' : %d - state may be inconsistent\n",
>  			pd->genpd.name, ret);

[Severity: High]
This is a pre-existing issue, but does ignoring this failure leave a dangling
pointer and cause a use-after-free?

During a probe error, scpsys_domain_cleanup() loops backwards by ID to tear
down domains. If the DT topology places a parent domain at a higher reg index
than its child, the loop will attempt to remove the parent first.

When this happens, pm_genpd_remove() will fail with -EBUSY because the child
is still linked (the driver doesn't explicitly unlink subdomains first).
Because the error is ignored here, devres will eventually free the
struct scpsys_domain memory while the domain is still linked in the global
gpd_list.

> -	if (scpsys_domain_is_on(pd))
> -		scpsys_power_off(&pd->genpd);
> +
> +	if (scpsys->soc_data->type == SCPSYS_MTCMOS_TYPE_HW_VOTER) {
> +		if (scpsys_hwv_domain_is_enable_done(pd))
> +			scpsys_hwv_power_off_internal(pd);
> +	} else {
> +		if (scpsys_domain_is_on(pd))
> +			scpsys_power_off_internal(pd);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but could this underflow the clock
and regulator refcounts during error cleanup?

For domains with MTK_SCPD_KEEP_DEFAULT_OFF, the driver skips calling power_on()
during initialization in scpsys_add_one_domain(). This means clocks and
regulators are never prepared or enabled by the driver.

If probe fails and the hardware was left ON by the bootloader, this check sees
the hardware is on and forcibly calls scpsys_hwv_power_off_internal() or
scpsys_power_off_internal(). This will unconditionally disable the clocks and
regulators, underflowing the refcounts for resources the driver never acquired.

>  
>  	clk_bulk_put(pd->num_clks, pd->clks);
>  	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709084635.24912-1-angelogioacchino.delregno@collabora.com?part=2