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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Preexisting race condition and ignored -EBUSY return from perf_pmu_unregister() leads to UAF if events are active during probe failure.
- [High] The global bitmask `cmask` is modified during device probe but not cleared on error paths, leading to a guaranteed NULL pointer dereference in exported functions.
- [High] The static key `sbi_pmu_snapshot_available` is never disabled on probe failure, leading to a potential NULL pointer dereference.
--

commit 4312c8eb8c51217703e557d54647da346fae9bf0
Author: Atish Patra <atishp@meta.com>
Subject: RISC-V: perf: fix resource cleanup on driver probe failure

This commit reorganizes the error handling paths in pmu_sbi_device_probe()
to properly free registered PMUs, per-cpu IRQs, the pmu_ctr_list, and
CPU hotplug states if the probe fails.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 385af5e6e6d0d..5c8924ce1f387 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
[ ... ]
> @@ -1515,9 +1525,20 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  	return 0;
>  
>  out_unregister:
> +	perf_pmu_unregister(&pmu->pmu);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this error path ignore a
potential -EBUSY return from perf_pmu_unregister()?

If perf_pmu_register() previously succeeded, an unprivileged user might race
to open an event on it. If a subsequent initialization step fails and triggers
this cleanup path, perf_pmu_unregister() can return -EBUSY and fail to remove
the PMU from the core structures.

Since the return value is ignored, the code will proceed to call kfree(pmu)
below. Can this leave a dangling pointer in the perf subsystem and result in a
use-after-free if those events are still active?

> +out_destroy:
>  	riscv_pmu_destroy(pmu);
> +	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;

[Severity: High]
This is a pre-existing issue, but does this error path leave the global cmask
variable set when pmu_ctr_list is freed?

During probe, pmu_sbi_get_ctrinfo() populates the global cmask. If the probe
fails and we take this error path, pmu_ctr_list is freed and set to NULL,
but cmask is never cleared.

If a subsequent call to the exported function riscv_pmu_get_hpm_info()
occurs (e.g., triggered by KVM vCPU initialization), could it bypass the
safety check because cmask is non-zero, and dereference the NULL pointer?

drivers/perf/riscv_pmu_sbi.c:riscv_pmu_get_hpm_info() {
    ...
    for_each_set_bit(i, &cmask, RISCV_MAX_COUNTERS) {
        info = &pmu_ctr_list[i];
        if (!info)
            continue;
    ...
}


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

Earlier in pmu_sbi_device_probe(), the static key is enabled:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_device_probe() {
    ...
    static_branch_enable(&sbi_pmu_snapshot_available);
    ...
}

If a subsequent step fails, the probe aborts and frees the snapshot memory,
but the static key remains enabled. If the driver is re-probed and setup
fails on the retry, the probe might continue successfully without snapshot
memory. Could this leave the static key true, causing functions like
pmu_sbi_ctr_read() to dereference a NULL snapshot_addr?

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_ctr_read() {
    ...
    struct riscv_pmu_snapshot_data *sdata = cpu_hw_evt->snapshot_addr;
    ...
    if (sbi_pmu_snapshot_available() && (hwc->state & PERF_HES_STOPPED)) {
        val = sdata->ctr_values[idx];
    ...
}

>  	kfree(pmu);
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-counter_delegation-v8-0-7909f863a645@meta.com?part=1