Re: [PATCH v7 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 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Premature PMU registration exposes the PMU to userspace before all initialization steps are complete, risking a Use-After-Free in the error path.
- [High] Global flag `riscv_pmu_use_irq` is not cleared when IRQ setup fails, causing a kernel panic during CPU hotplug initialization.
- [High] Probe failure leaves a dangling PMU snapshot physical page configured in the firmware for the boot CPU, leading to UAF and kernel memory corruption.
- [High] The CPU PM notifier is unregistered on error paths, but PMU structures are freed immediately without waiting for concurrent readers, leading to Use-After-Free.
--

commit e8a230ead53ac1c197ed8d75659a624a38133975
Author: Atish Patra <atishp@meta.com>

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

This commit reorganizes the error paths in pmu_sbi_device_probe() to
fix several resource cleanups. It ensures previously registered PMUs
and per-cpu IRQs are released appropriately on failure.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 385af5e6e6d0d..8753007cc57ec 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -1452,6 +1454,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  		pmu->pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
>  		pmu->pmu.capabilities |= PERF_PMU_CAP_NO_EXCLUDE;
>  	}
> +	irq_requested = (ret == 0);

[Severity: High]
This is a pre-existing issue, but when IRQ setup fails in
pmu_sbi_setup_irqs(), the global flag riscv_pmu_use_irq is not cleared.

Since the driver continues probing even if ret < 0 here, could this lead to a
kernel panic later during CPU hotplug?  When pmu_sbi_starting_cpu() runs, it
will check riscv_pmu_use_irq and call enable_percpu_irq(riscv_pmu_irq, ...).
Because the IRQ was never successfully requested, its desc->action is NULL,
which would cause a panic when handle_percpu_devid_irq() dereferences it.

[ ... ]

> @@ -1515,9 +1518,17 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  	return 0;
>  
>  out_unregister:
> +	perf_pmu_unregister(&pmu->pmu);

[Severity: Critical]
This isn't a bug introduced by this patch, but does ignoring the return value
of perf_pmu_unregister() here risk a use-after-free?

Because the PMU was registered earlier in the probe sequence, userspace might
have already opened a perf event by the time we hit an error (such as failing
to add the CPU hotplug instance).

If an event is active, perf_pmu_unregister() will return -EBUSY and leave the
PMU fully registered.

kernel/events/core.c:perf_pmu_unregister() {
    ...
    if (pmu->event_unmapped && !pmu_empty(pmu)) {
        ...
        return -EBUSY;
    }
    ...
}

Because the error path ignores the return value and proceeds to free the PMU
structures, userspace could continue interacting with the active perf events,
triggering a use-after-free of the PMU data structures.

> +
> +out_destroy:
>  	riscv_pmu_destroy(pmu);

[Severity: High]
This is a pre-existing issue, but does this error path properly handle PMU
snapshot cleanup on the boot CPU?

In pmu_sbi_device_probe(), the snapshot is manually configured for the boot
CPU using an SBI ecall inside pmu_sbi_snapshot_setup(), after which
preemption is enabled.

If a subsequent step fails, riscv_pmu_destroy() calls
pmu_sbi_snapshot_disable() which only disables the snapshot on the current
CPU. If the task migrated due to preemption, the snapshot would be disabled
on the wrong CPU. 

This would leave the boot CPU with a snapshot pointing to a physical page
that gets freed shortly after, allowing the firmware to silently overwrite
freed kernel memory.

[Severity: High]
This is also a pre-existing issue, but could this cause a use-after-free
during CPU PM transitions?

riscv_pmu_destroy() unregisters the CPU PM notifier using
riscv_pm_pmu_unregister(). Since cpu_pm_notifier is a raw notifier chain,
concurrent traversals during cpu_pm_enter() do not use RCU and do not block
unregistration.

Because the driver immediately calls kfree(pmu) and free_percpu() after this,
another CPU entering idle and traversing the notifier chain could dereference
the freed notifier_block and pmu struct.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-counter_delegation-v7-0-0ba2fd34614e@meta.com?part=1