Re: [PATCH v3] lib: sbi: pmu: Return SBI_EINVAL if cidx_mask is 0

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

On 5/13/25 6:40 AM, James Raphael Tiovalen wrote:
> Currently, when configuring a matching programmable HPM counter with
> Sscofpmf being present, cidx_base > 2, and cidx_mask == 0 to monitor
> either the CPU_CYCLES or INSTRUCTIONS hardware event,
> sbi_pmu_ctr_cfg_match will succeed but it will configure the
> corresponding fixed counter instead of the counter specified by the
> cidx_base parameter.
> 
> During counter configuration, the following issues may arise:
> - If the SKIP_MATCH flag is set, an out-of-bounds memory read of the
> phs->active_events array would occur, which could lead to undefined
> behavior.
> 
> - If the CLEAR_VALUE flag is set, the corresponding fixed counter will
> be reset, which could be considered unexpected behavior.
> 
> - If the AUTO_START flag is set, pmu_ctr_start_hw will silently start
> the fixed counter, even though it has already started. From the
> supervisor's perspective, nothing has changed, which could be confusing.
> The supervisor will not see the SBI_ERR_ALREADY_STARTED error code since
> sbi_pmu_ctr_cfg_match does not return the error code of
> pmu_ctr_start_hw.
> 
> The only way to detect these issues is to check the ctr_idx return value
> of sbi_pmu_ctr_cfg_match and compare it with cidx_base.
> 
> Fix these issues by returning the SBI_ERR_INVALID_PARAM error code if
> the cidx_mask parameter value being passed in is 0 since an invalid
> parameter should not lead to a successful sbi_pmu_ctr_cfg_match but with
> unexpected side effects.
> 
> Following a similar rationale, add the validation check to
> sbi_pmu_ctr_start and sbi_pmu_ctr_stop as well since sbi_fls is
> undefined when the mask is 0.
> 
> This also aligns OpenSBI's behavior with KVM's.
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>   lib/sbi/sbi_pmu.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index 5983a78..353a8f2 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -206,6 +206,12 @@ static int pmu_ctr_validate(struct sbi_pmu_hart_state *phs,
>   	return event_idx_type;
>   }
>   
> +static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask)
> +{
> +	/* Do a basic sanity check of counter base & mask */
> +	return cmask && ((cbase + sbi_fls(cmask)) < total_ctrs);
> +}
> +
>   int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
>   {
>   	int event_idx_type;
> @@ -472,7 +478,7 @@ int sbi_pmu_ctr_start(unsigned long cbase, unsigned long cmask,
>   	int i, cidx;
>   	uint64_t edata;
>   
> -	if ((cbase + sbi_fls(cmask)) >= total_ctrs)
> +	if (!pmu_ctr_idx_validate(cbase, cmask))
>   		return ret;
>   
>   	if (flags & SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT)
> @@ -577,8 +583,8 @@ int sbi_pmu_ctr_stop(unsigned long cbase, unsigned long cmask,
>   	uint32_t event_code;
>   	int i, cidx;
>   
> -	if ((cbase + sbi_fls(cmask)) >= total_ctrs)
> -		return SBI_EINVAL;
> +	if (!pmu_ctr_idx_validate(cbase, cmask))
> +		return ret;
>   
>   	if (flag & SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT)
>   		return SBI_ENO_SHMEM;
> @@ -839,8 +845,7 @@ int sbi_pmu_ctr_cfg_match(unsigned long cidx_base, unsigned long cidx_mask,
>   	int ret, event_type, ctr_idx = SBI_ENOTSUPP;
>   	u32 event_code;
>   
> -	/* Do a basic sanity check of counter base & mask */
> -	if ((cidx_base + sbi_fls(cidx_mask)) >= total_ctrs)
> +	if (!pmu_ctr_idx_validate(cidx_base, cidx_mask))
>   		return SBI_EINVAL;
>   
>   	event_type = pmu_event_validate(phs, event_idx, event_data);

Reviewed-by: Atish Patra <atishp@rivosinc.com>

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi