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

[Anup Patel]

On Tue, May 20, 2025 at 6:57 PM James Raphael Tiovalen
<jamestiotio at gmail.com> 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 at gmail.com>
> Reviewed-by: Atish Patra <atishp at rivosinc.com>
> Reviewed-by: Andrew Jones <ajones at ventanamicro.com>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  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..6ca4efd 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);
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi