Re: [PATCH v6 10/21] RISC-V: perf: Restructure the SBI PMU code

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] NULL pointer dereference when tearing down the PMU on error paths in probe.
- [High] Missing cleanup on error paths in rvpmu_device_probe() leads to Use-After-Free and resource leaks.
- [High] Broadcasting `SCOUNTEREN` updates to all CPUs in `mm_cpumask` leaks counter access to userspace.
--

commit 497e3337649afaedc330eca31dba2dda185d6179
Author: Atish Patra <atishp@rivosinc.com>

    RISC-V: perf: Restructure the SBI PMU code
    
    With Ssccfg/Smcdeleg, supervisor mode can program and access the hpmcounters
    and events directly, without the SBI PMU extension. The SBI PMU extension is
    still required for firmware counters. Restructure the existing SBI PMU code so
    the hpmcounter/event helpers can be shared between the SBI and the counter
    delegation paths that follow.

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

[ ... ]

> @@ -1305,7 +1349,7 @@ static void riscv_pmu_destroy(struct riscv_pmu *pmu)
>  	cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
>  }

[Severity: Critical]
This is a pre-existing issue, but does this code cause a NULL pointer
dereference on error paths?

In rvpmu_device_probe(), if an error occurs (such as perf_pmu_register(),
pmu_sbi_snapshot_alloc(), or cpuhp_state_add_instance() failing), the error
path jumps to out_unregister and calls riscv_pmu_destroy().

Because cpuhp_state_add_instance() either was not called yet, or failed and
did not hash the node, pmu->node.pprev is NULL. The function
cpuhp_state_remove_instance() does not check if the node is unhashed and
directly calls hlist_del(). This attempts to write to the previous node
pointer, which crashes the kernel with a NULL pointer dereference.

> -static void pmu_sbi_event_init(struct perf_event *event)
> +static void rvpmu_event_init(struct perf_event *event)

[ ... ]

> @@ -1347,14 +1391,14 @@ static void pmu_sbi_event_mapped(struct perf_event *event, struct mm_struct *mm)
>  	 * that it is possible to do so to avoid any race.
>  	 * And we must notify all cpus here because threads that currently run
>  	 * on other cpus will try to directly access the counter too without
> -	 * calling pmu_sbi_ctr_start.
> +	 * calling rvpmu_sbi_ctr_start.
>  	 */
>  	if (event->hw.flags & PERF_EVENT_FLAG_USER_ACCESS)
>  		on_each_cpu_mask(mm_cpumask(mm),
> -				 pmu_sbi_set_scounteren, (void *)event, 1);
> +				 rvpmu_set_scounteren, (void *)event, 1);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does broadcasting scounteren
updates to all CPUs leak counter access to userspace?

When a user-accessible event is mapped, rvpmu_set_scounteren is broadcast
across all CPUs where the process has threads running. This enables the
scounteren bit corresponding to event->hw.idx.

However, event->hw.idx is a dynamic, per-CPU hardware counter index allocated
only on the CPU where the event was scheduled. Broadcasting this index across
all CPUs in the mask grants userspace access to that specific hardware
counter slot on CPUs where the event is not running.

Could a thread on another CPU read from that counter slot, potentially leaking
the count of whatever unrelated perf event is active on that counter?

> -static void pmu_sbi_event_unmapped(struct perf_event *event, struct mm_struct *mm)
> +static void rvpmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)

[ ... ]

> @@ -1419,7 +1463,7 @@ static const struct ctl_table sbi_pmu_sysctl_table[] = {
>  	},
>  };
>  
> -static int pmu_sbi_device_probe(struct platform_device *pdev)
> +static int rvpmu_device_probe(struct platform_device *pdev)
>  {
>  	struct riscv_pmu *pmu = NULL;
>  	int ret = -ENODEV;

[Severity: High]
This is a pre-existing issue, but does this probe function correctly clean
up resources on error paths?

If initialization fails after perf_pmu_register(), the code jumps to the
out_unregister label:

out_unregister:
	riscv_pmu_destroy(pmu);

out_free:
	kfree(pmu);
	return ret;

However, riscv_pmu_destroy() does not call perf_pmu_unregister(). This leaves
the core perf subsystem with a dangling pointer to the freed pmu structure,
which can lead to a use-after-free when the perf core attempts to access it.

Additionally, the per-CPU memory allocated for pmu->hw_events and the per-CPU
interrupts requested via request_percpu_irq() are never freed on these error
paths, resulting in resource leaks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-counter_delegation-v6-0-285b72ed65a9@meta.com?part=10