Re: [PATCH v6 14/21] RISC-V: perf: Skip PMU SBI extension when not implemented

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing perf_pmu_unregister() on error paths leads to Use-After-Free of the pmu struct.
- [High] Unconditional cpuhp_state_remove_instance() on an uninitialized hlist_node causes a kernel panic.
--

commit bd51c5655be0902a2fc1fd90b8d692c299e7265b
Author: Charlie Jenkins <charlie@rivosinc.com>

RISC-V: perf: Skip PMU SBI extension when not implemented

When the PMU SBI extension is not implemented, sbi_v2_available should
not be set to true. The SBI implementation for counter config matching
and firmware counter read  should also be skipped when the SBI extension
is not implemented.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 1f16df9d0dd00..5bfcd3821f572 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
[ ... ]
> @@ -2045,12 +2052,13 @@ static int __init rvpmu_devinit(void)
>  	int ret;
>  	struct platform_device *pdev;
>  
> -	if (sbi_spec_version >= sbi_mk_version(0, 3) &&
> -	    sbi_probe_extension(SBI_EXT_PMU))
> -		static_branch_enable(&riscv_pmu_sbi_available);
> +	if (sbi_probe_extension(SBI_EXT_PMU)) {
> +		if (sbi_spec_version >= sbi_mk_version(0, 3))
> +			static_branch_enable(&riscv_pmu_sbi_available);
> +		if (sbi_spec_version >= sbi_mk_version(2, 0))
> +			sbi_v2_available = true;
> +	}
>  
> -	if (sbi_spec_version >= sbi_mk_version(2, 0))
> -		sbi_v2_available = true;
>  	/*
>  	 * We need all three extensions to be present to access the counters
>  	 * in S-mode via Supervisor Counter delegation.

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential use-after-free in the
error handling path of rvpmu_device_probe() when allocating or registering
fails after perf_pmu_register() succeeds.

If an error occurs, such as pmu_sbi_snapshot_alloc() failing, the code jumps
to out_unregister:

out_unregister:
	riscv_pmu_destroy(pmu);

out_free:
	kfree(pmu);
	return ret;

However, neither the error path nor riscv_pmu_destroy() calls
perf_pmu_unregister(&pmu->pmu).

This leaves the PMU registered in the perf core subsystem while its backing
memory is freed via kfree(). Any subsequent access to the PMU could result
in a use-after-free.

Should perf_pmu_unregister() be called during teardown to avoid memory
corruption?

[Severity: High]
This is a pre-existing issue, but the error teardown path in
rvpmu_device_probe() seems to have a bug that could trigger a kernel panic.

If rvpmu_device_probe() fails before successfully calling
cpuhp_state_add_instance(), it jumps to out_unregister and calls
riscv_pmu_destroy(pmu).

Inside riscv_pmu_destroy(), it unconditionally removes the hotplug instance:

static void riscv_pmu_destroy(struct riscv_pmu *pmu)
{
	...
	cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
}

Because the pmu struct is allocated with kzalloc() and the node hasn't been
added to the hotplug list yet, its hlist_node is zero-initialized.

When hlist_del() is called internally by cpuhp_state_remove_instance(), it
attempts to dereference node->pprev (which is NULL), causing a NULL pointer
dereference.

Would it be better to check if the node is actually hashed before attempting
to remove the hotplug instance?

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