Re: [PATCH 04/23] pmdomain: imx: fix OF node refcount

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Child platform devices dynamically created during probe are not unregistered on error or driver removal, leading to device leaks, memory leaks, and a use-after-free of the devres-managed regmap.
- [High] Power domains initialized as 'on' remain powered indefinitely due to missing `sync_state` callback and missing `GENPD_FLAG_NO_STAY_ON` flag.
- [High] Missing lower bound check on the signed `domain_index` variable can lead to an array out-of-bounds read.
- [High] A potential divide-by-zero kernel panic when calculating the power-off delay if the IPG clock rate is less than 1 MHz.
--

commit b02126ed3cd782919c6e44b786dd9f03cbf6fa28
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

pmdomain: imx: fix OF node refcount

This commit fixes a double put in platform_device_release() by adding
a missing call to of_node_get() when assigning the OF node to a
dynamically allocated platform device in imx_gpc_probe().

> diff --git a/drivers/pmdomain/imx/gpc.c b/drivers/pmdomain/imx/gpc.c
> index de695f1944ab3..42e50c9b4fb9f 100644
> --- a/drivers/pmdomain/imx/gpc.c
> +++ b/drivers/pmdomain/imx/gpc.c
> @@ -487,7 +487,7 @@ static int imx_gpc_probe(struct platform_device *pdev)

[Severity: High]
This is a pre-existing issue, but is there a missing sync_state
callback or GENPD_FLAG_NO_STAY_ON flag for these power domains?

The domains initialized as 'on' (with is_off = false in
pm_genpd_init()) have genpd->stay_on = true set by the genpd
subsystem. This flag prevents the domains from being powered off until
it is cleared.

Typically, a provider's sync_state callback clears this flag, but
neither imx_gpc_driver nor imx_pgc_power_domain_driver appear to
implement .sync_state. The driver also lacks the
GENPD_FLAG_NO_STAY_ON opt-out flag.

Could this cause unused power domains to remain powered on forever,
potentially leading to hardware issues when late regulator cleanups
disable regulators supplying these forced-on domains?

[Severity: High]
This isn't a bug introduced by this patch, but does the domain_index
validation adequately protect against negative values?

Earlier in the function, of_property_read_u32() reads a 32-bit
unsigned value from the device tree reg property into the memory of
domain_index, which is declared as a signed int. Input values
with the most significant bit set (>= 0x80000000) are interpreted
as negative numbers.

The upper bound check uses a signed comparison:

    if (domain_index >= of_id_data->num_domains)

which evaluates to false for negative numbers. Can this bypass the
validation and result in an out-of-bounds memory address computation
for &imx_gpc_domains[domain_index] when it is passed to
platform_device_add_data()?

>  			domain->ipg_rate_mhz = ipg_rate_mhz;

[Severity: High]
This is a pre-existing issue, but could the truncation of
ipg_rate_mhz lead to a divide-by-zero panic?

The ipg clock rate is calculated earlier using:

    ipg_rate_mhz = clk_get_rate(ipg_clk) / 1000000;

If the clock rate is less than 1,000,000 Hz, the integer division
truncates ipg_rate_mhz to 0.

Later, imx6_pm_domain_power_off() calculates a delay using:

    DIV_ROUND_UP(iso + iso2sw, pd->ipg_rate_mhz)

Since DIV_ROUND_UP(n, d) divides by the denominator, would passing
a denominator of 0 cause an immediate kernel panic when the system
attempts to turn off the power domain?

>  
>  			pd_pdev->dev.parent = &pdev->dev;
> -			pd_pdev->dev.of_node = np;
> +			pd_pdev->dev.of_node = of_node_get(np);
>  			pd_pdev->dev.fwnode = of_fwnode_handle(np);
>  
>  			ret = platform_device_add(pd_pdev);

[Severity: High]
This isn't a bug introduced by this patch, but are the dynamically
created child platform devices properly unregistered if
platform_device_add() fails?

The child platform devices (pd_pdev) are allocated and added within a
loop. If platform_device_add() fails on a subsequent iteration, the
function returns immediately without unregistering the previously added
child devices.

Similarly, imx_gpc_remove() does not unregister these devices when
the driver is unbound if the newer device tree binding is used (pgc_node
is present).

When the parent device is unbound or fails to probe, the devres
framework automatically frees its regmap. Because the leaked child
devices still hold a reference to this regmap (domain->regmap),
would any subsequent interaction with them trigger a use-after-free?

Furthermore, does the missing unregister path now permanently leak the
acquired of_node_get() references introduced in this patch?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-pdev-fwnode-ref-v1-0-88c324a1b8d2@oss.qualcomm.com?part=4