| Message ID | 20240724-fixes-v1-1-4a64596b0d64@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Add asserts for out-of-bound access | expand |
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
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
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
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
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
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 > >
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 > >
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;
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(+) --- base-commit: daff9f7f7a457f78ce455e6abf19c2a37dfe7630 change-id: 20240723-fixes-439b929bfbc8 -- Regards, Atish patra