Re: [PATCH] lib: sbi_pmu: fix integer overflow in pmu_ctr_idx_validate and cfg_match skip path

On Wed, Jun 24, 2026 at 9:21 AM liutong <[email protected]> wrote:
>
> pmu_ctr_idx_validate() checks whether counter indices are in range
> using cbase + sbi_fls(cmask) < total_ctrs.  Both operands are unsigned
> long, so a crafted cbase close to ULONG_MAX causes the addition to wrap
> around to a small value that passes the comparison.
>
> Once validation is bypassed, sbi_pmu_ctr_cfg_match() with the
> SKIP_MATCH flag uses the overflowed index directly as an array subscript
> into phs->active_events[], producing an out-of-bounds read in M-mode.
> Through the firmware-event code path, the same overflowed index reaches
> fw_counters_data[] and fw_counters_started, giving an attacker OOB
> write-zero and OOB bit-set primitives in M-mode memory.
>
> Fix pmu_ctr_idx_validate() by checking for unsigned overflow before the
> comparison, and add a secondary bounds check on cidx_first in the
> SKIP_MATCH path so that even if validation is somehow bypassed in the
> future, the array access remains bounded.
>
> Signed-off-by: liutong <[email protected]>

LGTM.

Reviewed-by: Anup Patel <[email protected]>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  lib/sbi/sbi_pmu.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index 480a9723..1b3e7c9a 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -224,8 +224,16 @@ static int pmu_ctr_validate(struct sbi_pmu_hart_state *phs,
>
>  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;
> +       unsigned long last;
> +
> +       if (!cmask)
> +               return false;
> +
> +       last = sbi_fls(cmask);
> +       if (cbase > ULONG_MAX - last)
> +               return false;
> +
> +       return (cbase + last) < total_ctrs;
>  }
>
>  int sbi_pmu_ctr_fw_read(unsigned long cidx, uint64_t *cval, bool high_bits)
> @@ -916,6 +924,9 @@ int sbi_pmu_ctr_cfg_match(unsigned long cidx_base, unsigned long cidx_mask,
>                 unsigned long cidx_first = cidx_base + sbi_ffs(cidx_mask);
>
> +               if (cidx_first >= total_ctrs)
> +                       return SBI_EINVAL;
> +
>                 if (phs->active_events[cidx_first] == SBI_PMU_EVENT_IDX_INVALID)
>                         return SBI_EINVAL;
>                 ctr_idx = cidx_first;
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/opensbi

-- 
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi