[PATCH 2/2] lib: sbi_pmu: reject out-of-bounds counter index in pmu_ctr_write_hw()

[Anup Patel]

On Sun, Oct 25, 2026 at 11:59 AM Sebastian Alba Vives
<sebasjosue84 at gmail.com> wrote:
>
> pmu_ctr_write_hw() writes CSR_MCYCLE + cidx with no check on cidx,
> while its sibling pmu_ctr_start_hw() guards the same index space:
>
>         if (cidx >= num_hw_ctrs || cidx == 1)
>                 return SBI_EINVAL;
>
> A cidx at or above num_hw_ctrs addresses an mhpmcounter the platform
> need not implement, and cidx == 1 addresses 0xb01, which is not
> assigned. Either one traps in M-mode instead of returning an error.
>
> Give the function the same guard and a return value. The guard is
> copied from pmu_ctr_start_hw() rather than limited to the upper
> bound, so that the two functions accept exactly the same index space.
>
> Only the call site in sbi_pmu_ctr_cfg_match() propagates the error.
> The two call sites inside pmu_ctr_start_hw() run after the identical
> guard in that function, so they cannot fail and are left unchanged.
>
> Fixes: 13d40f21d588 ("lib: sbi: Add PMU support")
> Signed-off-by: Sebastian Alba Vives <sebasjosue84 at gmail.com>

LGTM.

Reviewed-by: Anup Patel <anup at brainfault.org>

Regards,
Anup

> ---
>
> A note on reachability, so the Fixes tag is not read as more than it
> is.
>
> Since 9f37e25f ("lib: sbi: reject PMU skip-match counter class
> mismatches") the SKIP_MATCH path rejects a hardware event on a
> firmware counter. Index 1 cannot reach this call site either:
> active_events[1] is initialised to SBI_PMU_EVENT_IDX_INVALID and
> pmu_ctr_find_hw() masks out the fixed counters with
> ~SBI_PMU_FIXED_CTR_MASK. I could not find a route that gets a bad
> index into pmu_ctr_write_hw() on current master, so this is hardening
> of a latent defect, not a live one.
>
> I included cidx == 1 to match pmu_ctr_start_hw() exactly, which is
> slightly more than the out-of-bounds check you asked for. If you would
> rather the two functions guard different index spaces, I will resend
> with only the upper bound.
>
>  lib/sbi/sbi_pmu.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index 62d5e206..74aa2fe0 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -389,8 +389,12 @@ static int pmu_ctr_enable_irq_hw(int ctr_idx)
>         return 0;
>  }
>
> -static void pmu_ctr_write_hw(uint32_t cidx, uint64_t ival)
> +static int pmu_ctr_write_hw(uint32_t cidx, uint64_t ival)
>  {
> +       /* Make sure the counter index lies within the range and is not TM bit */
> +       if (cidx >= num_hw_ctrs || cidx == 1)
> +               return SBI_EINVAL;
> +
>  #if __riscv_xlen == 32
>         csr_write_num(CSR_MCYCLE + cidx, 0);
>         csr_write_num(CSR_MCYCLE + cidx, ival & 0xFFFFFFFF);
> @@ -398,6 +402,8 @@ static void pmu_ctr_write_hw(uint32_t cidx, uint64_t ival)
>  #else
>         csr_write_num(CSR_MCYCLE + cidx, ival);
>  #endif
> +
> +       return 0;
>  }
>
>  static int pmu_ctr_start_hw(uint32_t cidx, uint64_t ival, bool ival_update)
> @@ -980,8 +986,11 @@ skip_match:
>             event_type == SBI_PMU_EVENT_TYPE_HW_CACHE ||
>             event_type == SBI_PMU_EVENT_TYPE_HW_RAW ||
>             event_type == SBI_PMU_EVENT_TYPE_HW_RAW_V2) {
> -               if (flags & SBI_PMU_CFG_FLAG_CLEAR_VALUE)
> -                       pmu_ctr_write_hw(ctr_idx, 0);
> +               if (flags & SBI_PMU_CFG_FLAG_CLEAR_VALUE) {
> +                       ret = pmu_ctr_write_hw(ctr_idx, 0);
> +                       if (ret)
> +                               return ret;
> +               }
>                 if (flags & SBI_PMU_CFG_FLAG_AUTO_START) {
>                         ret = pmu_ctr_start_hw(ctr_idx, 0, false);
>                         if (ret && ret != SBI_EALREADY_STARTED)
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi