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

[Atish Patra <atish.patra@linux.dev>]

On 6/8/26 11:18 PM, sashiko-bot@kernel.org wrote:
> 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.

Good catch. Fixed in v7.

I will have a separate patch for these fixes.

>> -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?

It's a valid concern. Though it is very difficult to exploit to leak the 
counter values
through this approach as direct user access for programmable counters is 
a niche use case to begin with.

I think we can toggle the scounteren bit in add/delete path to avoid the 
leak. I will experiment
that idea and send a patch.

>> -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.
Nice catch. Fixed in v7.