Re: [PATCH v9 01/20] RISC-V: perf: fix resource cleanup on driver probe failure

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

Pre-existing issues:
- [Critical] Process migration during a probe failure leaves the SBI firmware configured to write to a freed memory page (Firmware Use-After-Free).
- [High] The `sbi_pmu_snapshot_available` static key is not disabled on the probe error path, leading to a potential NULL pointer dereference on subsequent probe attempts.
- [High] Race condition in the probe error path leads to Use-After-Free of the `pmu` structure.
- [Medium] Missing cleanup of the registered sysctl table on the probe error path.
- [Low] CPU hotplug state registered in init is not removed if subsequent init steps fail.
--

--- Patch [1]: [PATCH v9 01/20] RISC-V: perf: fix resource cleanup on driver probe failure ---
commit e501d85892f2097d5cb521e3b393610d4683e0a4
Author: Atish Patra <[email protected]>

RISC-V: perf: fix resource cleanup on driver probe failure

This patch fixes various UAF and memory leak issues around
pmu_sbi_device_probe() error paths. It ensures that registered PMUs,
per-cpu IRQs, and other data structures are correctly freed on failure.

Link: https://patch.msgid.link/[email protected]

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index dfc886dee5ad0..50220f7b46d9b 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -1301,7 +1308,8 @@ static void riscv_pmu_destroy(struct riscv_pmu *pmu)
>  		}
>  	}
>  	riscv_pm_pmu_unregister(pmu);
> -	cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
> +	if (!hlist_unhashed(&pmu->node))
> +		cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
>  }

[Severity: Critical]
This is a pre-existing issue, but does riscv_pmu_destroy() safely disable the
snapshot?

In pmu_sbi_device_probe(), pmu_sbi_snapshot_setup() is called within a
get_cpu()/put_cpu() block, configuring the SBI firmware for that specific CPU.

However, if probe fails and riscv_pmu_destroy() is called,
pmu_sbi_snapshot_disable() is executed without CPU pinning.

Could the thread migrate to a different CPU before disabling the snapshot,
leaving the original CPU's firmware configured to write to the snapshot
memory which is then freed by pmu_sbi_snapshot_free()?

[ ... ]

> @@ -1469,11 +1479,11 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  
>  	ret = riscv_pm_pmu_register(pmu);
>  	if (ret)
> -		goto out_unregister;
> +		goto out_destroy;
>  
>  	ret = perf_pmu_register(&pmu->pmu, "cpu", PERF_TYPE_RAW);
>  	if (ret)
> -		goto out_unregister;
> +		goto out_destroy;

[Severity: High]
This is a pre-existing issue, but does this expose the PMU to userspace before
fallible setups are complete?

Since perf_pmu_register() makes the PMU accessible, if userspace opens a perf
event before a subsequent initialization step fails (such as allocating
snapshot memory), the driver jumps to out_unregister and eventually calls
kfree(pmu).

Will this leave the perf core holding a pointer to a freed pmu structure?

[ ... ]

> @@ -1514,9 +1524,20 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  	return 0;
>  
>  out_unregister:
> +	perf_pmu_unregister(&pmu->pmu);
> +
> +out_destroy:

[Severity: Medium]
This is a pre-existing issue, but is the sysctl table leaked on this error
path?

Before the return 0, pmu_sbi_device_probe() calls:

    register_sysctl("kernel", sbi_pmu_sysctl_table);

If the subsequent cpuhp_state_add_instance() call fails, the error path skips
unregistering this sysctl table. Will repeated probe failures leak
ctl_table_header structures?

>  	riscv_pmu_destroy(pmu);

[Severity: High]
This is a pre-existing issue, but does the error path fail to disable the
sbi_pmu_snapshot_available static key?

If the probe fails after static_branch_enable(&sbi_pmu_snapshot_available),
the error path frees the snapshot memory in riscv_pmu_destroy() but never
disables the static key.

On a subsequent probe attempt or an interrupt, could code paths like
pmu_sbi_ovf_handler() execute assuming the snapshot memory is active, leading
to a NULL pointer dereference when accessing sdata->ctr_overflow_mask?

> +	if (irq_requested) {
> +		free_percpu_irq(riscv_pmu_irq, pmu->hw_events);
> +		irq_dispose_mapping(riscv_pmu_irq);
> +		riscv_pmu_irq = 0;
> +	}
>  
>  out_free:
> +	free_percpu(pmu->hw_events);
> +	kfree(pmu_ctr_list);
> +	pmu_ctr_list = NULL;
>  	kfree(pmu);
>  	return ret;
>  }

[Severity: Low]
This is a pre-existing issue, but is there a CPU hotplug state leak in
pmu_sbi_devinit() if platform registration fails?

pmu_sbi_devinit() registers a multi-state CPU hotplug hook:

    ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_RISCV_STARTING, ...);

If platform_driver_register() or platform_device_register_simple() fails
afterward, the function returns directly without calling
cpuhp_remove_multi_state(), leaving the hotplug state registered.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1