Re: [PATCH v2 4/4] perf/arm_cspmu: Make IRQ more optional

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional swallowing of `arm_cspmu_request_irq()` error codes (such as `-EPROBE_DEFER` and `-ENOMEM`) for PMUs with >= 64-bit counters.

Pre-existing issues:
- [High] Incorrect ordering between CPU hotplug state management and PMU registration causes Use-After-Free during teardown.
- [High] Missing cleanup of CPU hotplug state if `platform_driver_register()` fails in module init.
- [High] Failed `driver_attach()` in `arm_cspmu_impl_register()` leaves dangling pointers in the `impl_match` array.
- [High] Stack-allocated bitmap cast to `u32 *` leaves memory uninitialized on Big Endian systems, causing reads of uninitialized stack memory.
--

--- Patch [4]: [PATCH v2 4/4] perf/arm_cspmu: Make IRQ more optional ---
commit e7afdbca83ffd562889abdf4a15a47f1af9fc395
Author: Robin Murphy <robin.murphy@arm.com>

perf/arm_cspmu: Make IRQ more optional

This commit allows the driver to continue probing even if it fails to
request an IRQ, provided the PMU has 64-bit counters. It assumes that
64-bit counters will not overflow in a realistic timeframe.

> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 385d3e34694f2..007038aa7e9a6 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -1256,8 +1256,11 @@ static int arm_cspmu_device_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	ret = arm_cspmu_request_irq(cspmu);
> -	if (ret)
> -		return ret;
> +	if (ret) {
> +		if (counter_size(cspmu) < 64)
> +			return ret;

[Severity: High]
Does this unintentionally swallow error codes like -EPROBE_DEFER or
-ENOMEM for 64-bit PMUs?

If the interrupt controller is not yet ready, arm_cspmu_request_irq() will
return -EPROBE_DEFER via platform_get_irq_optional(). By completely ignoring
the error here, does this result in completing the probe without an IRQ when
we actually just needed to defer it?

Similarly, memory allocation failures (-ENOMEM) from devm_request_irq()
would also be silently ignored.

> +		dev_info(cspmu->dev, "Continuing without IRQ\n");
> +	}
>  
>  	ret = arm_cspmu_get_cpus(cspmu);

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
window during teardown if the PMU is unregistered before the hotplug state
is removed?

In arm_cspmu_device_remove():

    perf_pmu_unregister(&cspmu->pmu);
    cpuhp_state_remove_instance(arm_cspmu_cpuhp_state, &cspmu->cpuhp_node);

Calling perf_pmu_unregister() frees the PMU context. When
cpuhp_state_remove_instance() is called next, it synchronously triggers
arm_cspmu_cpu_teardown() on all online CPUs.

The teardown callback then invokes perf_pmu_migrate_context() using the
already freed PMU context.

A similar sequence appears in the error path of arm_cspmu_register_pmu() if
perf_pmu_register() fails.

[Severity: High]
This isn't a bug introduced by this patch, but does arm_cspmu_init() leak
the CPU hotplug callbacks if platform_driver_register() fails?

In arm_cspmu_init():

    arm_cspmu_cpuhp_state = ret;
    return platform_driver_register(&arm_cspmu_driver);

If platform_driver_register() fails, the module memory is freed. However,
cpuhp_remove_multi_state() is missing from this error path, which leaves
the kernel holding pointers to the freed module callbacks.

Can this cause a kernel panic on subsequent CPU hotplug events?

[Severity: High]
This isn't a bug introduced by this patch, but does arm_cspmu_impl_register()
leave dangling pointers in the static impl_match array on failure?

In arm_cspmu_impl_register():

    if (!match->impl_init_ops) {
        match->module = impl_match->module;
        match->impl_init_ops = impl_match->impl_init_ops;
    }
    ...
    if (!ret)
        ret = driver_attach(&arm_cspmu_driver.driver);

If driver_attach() fails, the function returns an error without clearing
the module and impl_init_ops fields. If the backend module is unloaded,
these pointers become invalid.

Would a subsequent probe of a matching device call the dangling
impl_init_ops pointer?

[Severity: High]
This isn't a bug introduced by this patch, but can the uninitialized stack
bitmap cause logic failures on Big Endian systems?

In arm_cspmu_handle_irq(), pmovs is declared on the stack:

    DECLARE_BITMAP(pmovs, ARM_CSPMU_MAX_HW_CNTRS);

And in arm_cspmu_get_reset_overflow(), it is written to as a u32 array:

    for (i = 0; i < cspmu->num_set_clr_reg; ++i) {
        pmovs[i] = readl(cspmu->base1 + pmovclr_offset);

Writing a u32 sequentially into an unsigned long array on a 64-bit Big
Endian system populates the most-significant 32 bits, leaving the
least-significant 32 bits uninitialized.

Since test_bit() checks bits based on unsigned long layout, could it read
from the uninitialized stack memory, causing spurious or missed hardware IRQs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784213435.git.robin.murphy@arm.com?part=4