[PATCH] target/riscv: Add asserts for out-of-bound access

[PATCH] target/riscv: Add asserts for out-of-bound access

[Atish Patra]

Coverity complained about the possible out-of-bounds access with
counter_virt/counter_virt_prev because these two arrays are
accessed with privilege mode. However, these two arrays are accessed
only when virt is enabled. Thus, the privilege mode can't be M mode.

Add the asserts anyways to detect any wrong usage of these arrays
in the future.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
The lore discussion can be found here
https://lore.kernel.org/all/CAHBxVyGQHBobpf71o4Qp51iQGXKBh0Ajup=e_a95xdLF==V_WQ@mail.gmail.com/
---
 target/riscv/pmu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 3cc0b3648cad..e05ab067d2f2 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -204,6 +204,7 @@ static void riscv_pmu_icount_update_priv(CPURISCVState *env,
     }
 
     if (env->virt_enabled) {
+        g_assert(env->priv <= PRV_S);
         counter_arr = env->pmu_fixed_ctrs[1].counter_virt;
         snapshot_prev = env->pmu_fixed_ctrs[1].counter_virt_prev;
     } else {
@@ -212,6 +213,7 @@ static void riscv_pmu_icount_update_priv(CPURISCVState *env,
     }
 
     if (new_virt) {
+        g_assert(newpriv <= PRV_S);
         snapshot_new = env->pmu_fixed_ctrs[1].counter_virt_prev;
     } else {
         snapshot_new = env->pmu_fixed_ctrs[1].counter_prev;
@@ -242,6 +244,7 @@ static void riscv_pmu_cycle_update_priv(CPURISCVState *env,
     }
 
     if (env->virt_enabled) {
+        g_assert(env->priv <= PRV_S);
         counter_arr = env->pmu_fixed_ctrs[0].counter_virt;
         snapshot_prev = env->pmu_fixed_ctrs[0].counter_virt_prev;
     } else {
@@ -250,6 +253,7 @@ static void riscv_pmu_cycle_update_priv(CPURISCVState *env,
     }
 
     if (new_virt) {
+        g_assert(newpriv <= PRV_S);
         snapshot_new = env->pmu_fixed_ctrs[0].counter_virt_prev;
     } else {
         snapshot_new = env->pmu_fixed_ctrs[0].counter_prev;

---
base-commit: daff9f7f7a457f78ce455e6abf19c2a37dfe7630
change-id: 20240723-fixes-439b929bfbc8
--
Regards,
Atish patra

Re: [PATCH] target/riscv: Add asserts for out-of-bound access

[Alistair Francis]

On Wed, Jul 24, 2024 at 6:33 PM Atish Patra <atishp@rivosinc.com> wrote:
>
> Coverity complained about the possible out-of-bounds access with
> counter_virt/counter_virt_prev because these two arrays are
> accessed with privilege mode. However, these two arrays are accessed
> only when virt is enabled. Thus, the privilege mode can't be M mode.
>
> Add the asserts anyways to detect any wrong usage of these arrays
> in the future.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Fixes: Coverity CID 1558459
Fixes: Coverity CID 1558462

> ---
> The lore discussion can be found here
> https://lore.kernel.org/all/CAHBxVyGQHBobpf71o4Qp51iQGXKBh0Ajup=e_a95xdLF==V_WQ@mail.gmail.com/
> ---
>  target/riscv/pmu.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 3cc0b3648cad..e05ab067d2f2 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -204,6 +204,7 @@ static void riscv_pmu_icount_update_priv(CPURISCVState *env,
>      }
>
>      if (env->virt_enabled) {
> +        g_assert(env->priv <= PRV_S);

Don't we need this assert for !env->virt_enabled as well?

Alistair

Re: [PATCH] target/riscv: Add asserts for out-of-bound access

[Atish Kumar Patra]

On Thu, Jul 25, 2024 at 10:12 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jul 24, 2024 at 6:33 PM Atish Patra <atishp@rivosinc.com> wrote:
> >
> > Coverity complained about the possible out-of-bounds access with
> > counter_virt/counter_virt_prev because these two arrays are
> > accessed with privilege mode. However, these two arrays are accessed
> > only when virt is enabled. Thus, the privilege mode can't be M mode.
> >
> > Add the asserts anyways to detect any wrong usage of these arrays
> > in the future.
> >
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
>
> Fixes: Coverity CID 1558459
> Fixes: Coverity CID 1558462
>
> > ---
> > The lore discussion can be found here
> > https://lore.kernel.org/all/CAHBxVyGQHBobpf71o4Qp51iQGXKBh0Ajup=e_a95xdLF==V_WQ@mail.gmail.com/
> > ---
> >  target/riscv/pmu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index 3cc0b3648cad..e05ab067d2f2 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -204,6 +204,7 @@ static void riscv_pmu_icount_update_priv(CPURISCVState *env,
> >      }
> >
> >      if (env->virt_enabled) {
> > +        g_assert(env->priv <= PRV_S);
>
> Don't we need this assert for !env->virt_enabled as well?
>

For that case, it uses counter and counter_prev which is array size of 4.
The assert was in the other case just to avoid wrong invocation in the
future with PRV_M while the array size is 2.

> Alistair

Re: [PATCH] target/riscv: Add asserts for out-of-bound access

[Atish Kumar Patra]

On Thu, Jul 25, 2024 at 10:12 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jul 24, 2024 at 6:33 PM Atish Patra <atishp@rivosinc.com> wrote:
> >
> > Coverity complained about the possible out-of-bounds access with
> > counter_virt/counter_virt_prev because these two arrays are
> > accessed with privilege mode. However, these two arrays are accessed
> > only when virt is enabled. Thus, the privilege mode can't be M mode.
> >
> > Add the asserts anyways to detect any wrong usage of these arrays
> > in the future.
> >
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
>
> Fixes: Coverity CID 1558459
> Fixes: Coverity CID 1558462
>

I think one of the Coverity issues was about the get_field issue in
the other thread?
This doesn't necessarily fix the coverity issue also as the issue
reported is a false positive.
But I don't mind citing the coverity issues as it is reported by that.

Is there a link to both coverity issues to know which issue describes
the out-of-bound access one ?

> > ---
> > The lore discussion can be found here
> > https://lore.kernel.org/all/CAHBxVyGQHBobpf71o4Qp51iQGXKBh0Ajup=e_a95xdLF==V_WQ@mail.gmail.com/
> > ---
> >  target/riscv/pmu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index 3cc0b3648cad..e05ab067d2f2 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -204,6 +204,7 @@ static void riscv_pmu_icount_update_priv(CPURISCVState *env,
> >      }
> >
> >      if (env->virt_enabled) {
> > +        g_assert(env->priv <= PRV_S);
>
> Don't we need this assert for !env->virt_enabled as well?
>
> Alistair

Re: [PATCH] target/riscv: Add asserts for out-of-bound access

[Alistair Francis]

On Sat, Jul 27, 2024 at 11:36 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Thu, Jul 25, 2024 at 10:12 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jul 24, 2024 at 6:33 PM Atish Patra <atishp@rivosinc.com> wrote:
> > >
> > > Coverity complained about the possible out-of-bounds access with
> > > counter_virt/counter_virt_prev because these two arrays are
> > > accessed with privilege mode. However, these two arrays are accessed
> > > only when virt is enabled. Thus, the privilege mode can't be M mode.
> > >
> > > Add the asserts anyways to detect any wrong usage of these arrays
> > > in the future.
> > >
> > > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >
> > Fixes: Coverity CID 1558459
> > Fixes: Coverity CID 1558462
> >
>
> I think one of the Coverity issues was about the get_field issue in
> the other thread?
> This doesn't necessarily fix the coverity issue also as the issue
> reported is a false positive.
> But I don't mind citing the coverity issues as it is reported by that.
>
> Is there a link to both coverity issues to know which issue describes
> the out-of-bound access one ?

I don't think so. I can see the report though and I think it should be
both of them. They are hard to read, but they both seem relevant.

1558462 is the confusing one, but it has

CID 1558462:  Memory - corruptions  (OVERRUN)
>>>     Overrunning callee's array of size 2 by passing argument "env->priv" (which evaluates to 2) in call to "riscv_cpu_set_mode".

so I think this should fix it

Alistair

Re: [PATCH] target/riscv: Add asserts for out-of-bound access

[Peter Maydell]

On Sat, 27 Jul 2024 at 02:36, Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Thu, Jul 25, 2024 at 10:12 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jul 24, 2024 at 6:33 PM Atish Patra <atishp@rivosinc.com> wrote:
> > >
> > > Coverity complained about the possible out-of-bounds access with
> > > counter_virt/counter_virt_prev because these two arrays are
> > > accessed with privilege mode. However, these two arrays are accessed
> > > only when virt is enabled. Thus, the privilege mode can't be M mode.
> > >
> > > Add the asserts anyways to detect any wrong usage of these arrays
> > > in the future.
> > >
> > > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >
> > Fixes: Coverity CID 1558459
> > Fixes: Coverity CID 1558462
> >
>
> I think one of the Coverity issues was about the get_field issue in
> the other thread?
> This doesn't necessarily fix the coverity issue also as the issue
> reported is a false positive.
> But I don't mind citing the coverity issues as it is reported by that.
>
> Is there a link to both coverity issues to know which issue describes
> the out-of-bound access one ?

You can't do deep links into the Coverity Scan UI, but if you
want to ask for an account at https://scan.coverity.com/projects/qemu
we generally give them to any developer who asks.

In this case 1558459 is complaining about the call to
     riscv_pmu_update_fixed_ctrs(env, newpriv, virt_en);
in riscv_cpu_set_mode(), and 1558462 is complaining about the call to
 riscv_cpu_set_mode(env, PRV_M, virt)
in riscv_cpu_do_interrupt().

So it's basically reported the same problem twice, at different
levels in the callstack. I don't know why it's done that, but it's
not that uncommon that the same problem gets detected several ways.

The complaints about get_field/set_field were different: those were
1558461, 1558463. I've already marked those as false-positives in the UI.

(Generally my practice is that where we think there's no point in
making a change to QEMU's code I mark the issue as a false-positive;
where we think it is reasonable to make a change to QEMU's code
(e.g. to aid the human reader or where we think an assert() is useful)
I leave it marked as "bug" to indicate we want to change something,
even if Coverity's analysis is wrong and it's a can't-happen case.)

thanks
-- PMM

Re: [PATCH] target/riscv: Add asserts for out-of-bound access

[Alistair Francis]

On Wed, Jul 24, 2024 at 6:33 PM Atish Patra <atishp@rivosinc.com> wrote:
>
> Coverity complained about the possible out-of-bounds access with
> counter_virt/counter_virt_prev because these two arrays are
> accessed with privilege mode. However, these two arrays are accessed
> only when virt is enabled. Thus, the privilege mode can't be M mode.
>
> Add the asserts anyways to detect any wrong usage of these arrays
> in the future.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> The lore discussion can be found here
> https://lore.kernel.org/all/CAHBxVyGQHBobpf71o4Qp51iQGXKBh0Ajup=e_a95xdLF==V_WQ@mail.gmail.com/
> ---
>  target/riscv/pmu.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 3cc0b3648cad..e05ab067d2f2 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -204,6 +204,7 @@ static void riscv_pmu_icount_update_priv(CPURISCVState *env,
>      }
>
>      if (env->virt_enabled) {
> +        g_assert(env->priv <= PRV_S);
>          counter_arr = env->pmu_fixed_ctrs[1].counter_virt;
>          snapshot_prev = env->pmu_fixed_ctrs[1].counter_virt_prev;
>      } else {
> @@ -212,6 +213,7 @@ static void riscv_pmu_icount_update_priv(CPURISCVState *env,
>      }
>
>      if (new_virt) {
> +        g_assert(newpriv <= PRV_S);
>          snapshot_new = env->pmu_fixed_ctrs[1].counter_virt_prev;
>      } else {
>          snapshot_new = env->pmu_fixed_ctrs[1].counter_prev;
> @@ -242,6 +244,7 @@ static void riscv_pmu_cycle_update_priv(CPURISCVState *env,
>      }
>
>      if (env->virt_enabled) {
> +        g_assert(env->priv <= PRV_S);
>          counter_arr = env->pmu_fixed_ctrs[0].counter_virt;
>          snapshot_prev = env->pmu_fixed_ctrs[0].counter_virt_prev;
>      } else {
> @@ -250,6 +253,7 @@ static void riscv_pmu_cycle_update_priv(CPURISCVState *env,
>      }
>
>      if (new_virt) {
> +        g_assert(newpriv <= PRV_S);
>          snapshot_new = env->pmu_fixed_ctrs[0].counter_virt_prev;
>      } else {
>          snapshot_new = env->pmu_fixed_ctrs[0].counter_prev;
>
> ---
> base-commit: daff9f7f7a457f78ce455e6abf19c2a37dfe7630
> change-id: 20240723-fixes-439b929bfbc8
> --
> Regards,
> Atish patra
>
>

Re: [PATCH] target/riscv: Add asserts for out-of-bound access

[Alistair Francis]

On Wed, Jul 24, 2024 at 6:33 PM Atish Patra <atishp@rivosinc.com> wrote:
>
> Coverity complained about the possible out-of-bounds access with
> counter_virt/counter_virt_prev because these two arrays are
> accessed with privilege mode. However, these two arrays are accessed
> only when virt is enabled. Thus, the privilege mode can't be M mode.
>
> Add the asserts anyways to detect any wrong usage of these arrays
> in the future.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
> The lore discussion can be found here
> https://lore.kernel.org/all/CAHBxVyGQHBobpf71o4Qp51iQGXKBh0Ajup=e_a95xdLF==V_WQ@mail.gmail.com/
> ---
>  target/riscv/pmu.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 3cc0b3648cad..e05ab067d2f2 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -204,6 +204,7 @@ static void riscv_pmu_icount_update_priv(CPURISCVState *env,
>      }
>
>      if (env->virt_enabled) {
> +        g_assert(env->priv <= PRV_S);
>          counter_arr = env->pmu_fixed_ctrs[1].counter_virt;
>          snapshot_prev = env->pmu_fixed_ctrs[1].counter_virt_prev;
>      } else {
> @@ -212,6 +213,7 @@ static void riscv_pmu_icount_update_priv(CPURISCVState *env,
>      }
>
>      if (new_virt) {
> +        g_assert(newpriv <= PRV_S);
>          snapshot_new = env->pmu_fixed_ctrs[1].counter_virt_prev;
>      } else {
>          snapshot_new = env->pmu_fixed_ctrs[1].counter_prev;
> @@ -242,6 +244,7 @@ static void riscv_pmu_cycle_update_priv(CPURISCVState *env,
>      }
>
>      if (env->virt_enabled) {
> +        g_assert(env->priv <= PRV_S);
>          counter_arr = env->pmu_fixed_ctrs[0].counter_virt;
>          snapshot_prev = env->pmu_fixed_ctrs[0].counter_virt_prev;
>      } else {
> @@ -250,6 +253,7 @@ static void riscv_pmu_cycle_update_priv(CPURISCVState *env,
>      }
>
>      if (new_virt) {
> +        g_assert(newpriv <= PRV_S);
>          snapshot_new = env->pmu_fixed_ctrs[0].counter_virt_prev;
>      } else {
>          snapshot_new = env->pmu_fixed_ctrs[0].counter_prev;
>
> ---
> base-commit: daff9f7f7a457f78ce455e6abf19c2a37dfe7630
> change-id: 20240723-fixes-439b929bfbc8
> --
> Regards,
> Atish patra
>
>