Re: [PULL 21/30] target/riscv: Implement privilege mode filtering for cycle/instret

[Peter Maydell]

On Thu, 18 Jul 2024 at 03:12, Alistair Francis <alistair23@gmail.com> wrote:
>
> From: Atish Patra <atishp@rivosinc.com>
>
> Privilege mode filtering can also be emulated for cycle/instret by
> tracking host_ticks/icount during each privilege mode switch. This
> patch implements that for both cycle/instret and mhpmcounters. The
> first one requires Smcntrpmf while the other one requires Sscofpmf
> to be enabled.
>
> The cycle/instret are still computed using host ticks when icount
> is not enabled. Otherwise, they are computed using raw icount which
> is more accurate in icount mode.

Hi; Coverity points out some possible issues with this patch
(CID 1558459, 1558462):


> +typedef struct PMUFixedCtrState {
> +        /* Track cycle and icount for each privilege mode */
> +        uint64_t counter[4];
> +        uint64_t counter_prev[4];
> +        /* Track cycle and icount for each privilege mode when V = 1*/
> +        uint64_t counter_virt[2];
> +        uint64_t counter_virt_prev[2];

These two arrays are defined with size 2...


> +static void riscv_pmu_icount_update_priv(CPURISCVState *env,
> +                                         target_ulong newpriv, bool new_virt)
> +{
> +    uint64_t *snapshot_prev, *snapshot_new;
> +    uint64_t current_icount;
> +    uint64_t *counter_arr;
> +    uint64_t delta;
> +
> +    if (icount_enabled()) {
> +        current_icount = icount_get_raw();
> +    } else {
> +        current_icount = cpu_get_host_ticks();
> +    }
> +
> +    if (env->virt_enabled) {
> +        counter_arr = env->pmu_fixed_ctrs[1].counter_virt;
> +        snapshot_prev = env->pmu_fixed_ctrs[1].counter_virt_prev;
> +    } else {
> +        counter_arr = env->pmu_fixed_ctrs[1].counter;
> +        snapshot_prev = env->pmu_fixed_ctrs[1].counter_prev;
> +    }
> +
> +    if (new_virt) {
> +        snapshot_new = env->pmu_fixed_ctrs[1].counter_virt_prev;
> +    } else {
> +        snapshot_new = env->pmu_fixed_ctrs[1].counter_prev;
> +    }
> +
> +     /*
> +      * new_priv can be same as env->priv. So we need to calculate
> +      * delta first before updating snapshot_new[new_priv].
> +      */
> +    delta = current_icount - snapshot_prev[env->priv];
> +    snapshot_new[newpriv] = current_icount;
> +
> +    counter_arr[env->priv] += delta;

...and in this function we may use those counter_virt and
counter_virt_prev arrays with newpriv and env->priv as indexes,
but in the callsites like riscv_cpu_set_mode() the assertions on
newpriv etc are things like
  g_assert(newpriv <= PRV_M && newpriv != PRV_RESERVED);

so Coverity thinks newpriv might be PRV_M (because that's the only
explicit range guard it's seen) and we will overrun the array.

If this is a "can't happen" case I think we should have asserts in the
functions like riscv_pmu_icount_update_priv() and riscv_pmu_cycle_update_priv()
that might index into these arrays that the indexes can't be out of bounds
for these smaller arrays.

thanks
-- PMM