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

[James R T]

On Tue, May 20, 2025 at 11:37 PM Andrew Jones <ajones at ventanamicro.com> wrote:
>
> On Tue, May 20, 2025 at 09:25:33PM +0800, 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 at gmail.com>
> > Reviewed-by: Atish Patra <atishp at rivosinc.com>
> > Reviewed-by: Andrew Jones <ajones at ventanamicro.com>
> > ---
>
> Need a changelog here. I see the only change from v3 is dropping the (),
> though.
>

For posterity, the complete changelog is:
- v2: Improve commit message with bug description
- v3: Add check to sbi_pmu_ctr_start and sbi_pmu_ctr_stop
- v4: Remove unnecessary parentheses

Best regards,
James Raphael Tiovalen

> Thanks,
> drew
>
> >  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
> >