[PATCH] target/xtensa: Assert that interrupt level is within bounds

[PATCH] target/xtensa: Assert that interrupt level is within bounds

[Peter Maydell]

In handle_interrupt() we use level as an index into the interrupt_vector[]
array. This is safe because we have checked it against env->config->nlevel,
but Coverity can't see that (and it is only true because each CPU config
sets its XCHAL_NUM_INTLEVELS to something less than MAX_NLEVELS), so it
complains about a possible array overrun (CID 1507131)

Add an assert() which will make Coverity happy and catch the unlikely
case of a mis-set XCHAL_NUM_INTLEVELS in future.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
NB: only tested with 'make check-avocado'. You could argue that we
should mark the coverity issue as false-positive instead if you like.
---
 target/xtensa/exc_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/xtensa/exc_helper.c b/target/xtensa/exc_helper.c
index d4823a65cda..43f6a862de2 100644
--- a/target/xtensa/exc_helper.c
+++ b/target/xtensa/exc_helper.c
@@ -169,6 +169,9 @@ static void handle_interrupt(CPUXtensaState *env)
         CPUState *cs = env_cpu(env);
 
         if (level > 1) {
+            /* env->config->nlevel check should have ensured this */
+            assert(level < sizeof(env->config->interrupt_vector));
+
             env->sregs[EPC1 + level - 1] = env->pc;
             env->sregs[EPS2 + level - 2] = env->sregs[PS];
             env->sregs[PS] =
-- 
2.34.1

Re: [PATCH] target/xtensa: Assert that interrupt level is within bounds

[Max Filippov]

On Fri, Jun 23, 2023 at 8:41 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In handle_interrupt() we use level as an index into the interrupt_vector[]
> array. This is safe because we have checked it against env->config->nlevel,
> but Coverity can't see that (and it is only true because each CPU config
> sets its XCHAL_NUM_INTLEVELS to something less than MAX_NLEVELS), so it
> complains about a possible array overrun (CID 1507131)
>
> Add an assert() which will make Coverity happy and catch the unlikely
> case of a mis-set XCHAL_NUM_INTLEVELS in future.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> NB: only tested with 'make check-avocado'. You could argue that we
> should mark the coverity issue as false-positive instead if you like.
> ---
>  target/xtensa/exc_helper.c | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

Re: [PATCH] target/xtensa: Assert that interrupt level is within bounds

[Peter Maydell]

On Sat, 24 Jun 2023 at 01:20, Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> On Fri, Jun 23, 2023 at 8:41 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > In handle_interrupt() we use level as an index into the interrupt_vector[]
> > array. This is safe because we have checked it against env->config->nlevel,
> > but Coverity can't see that (and it is only true because each CPU config
> > sets its XCHAL_NUM_INTLEVELS to something less than MAX_NLEVELS), so it
> > complains about a possible array overrun (CID 1507131)
> >
> > Add an assert() which will make Coverity happy and catch the unlikely
> > case of a mis-set XCHAL_NUM_INTLEVELS in future.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > NB: only tested with 'make check-avocado'. You could argue that we
> > should mark the coverity issue as false-positive instead if you like.
> > ---
> >  target/xtensa/exc_helper.c | 3 +++
> >  1 file changed, 3 insertions(+)
>
> Acked-by: Max Filippov <jcmvbkbc@gmail.com>

Thanks. I'll take it via target-arm since I'm doing a pullreq
anyway, unless you'd prefer otherwise.

-- PMM

Re: [PATCH] target/xtensa: Assert that interrupt level is within bounds

[Max Filippov]

On Tue, Jul 4, 2023 at 6:27 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 24 Jun 2023 at 01:20, Max Filippov <jcmvbkbc@gmail.com> wrote:
> >
> > On Fri, Jun 23, 2023 at 8:41 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > In handle_interrupt() we use level as an index into the interrupt_vector[]
> > > array. This is safe because we have checked it against env->config->nlevel,
> > > but Coverity can't see that (and it is only true because each CPU config
> > > sets its XCHAL_NUM_INTLEVELS to something less than MAX_NLEVELS), so it
> > > complains about a possible array overrun (CID 1507131)
> > >
> > > Add an assert() which will make Coverity happy and catch the unlikely
> > > case of a mis-set XCHAL_NUM_INTLEVELS in future.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > > NB: only tested with 'make check-avocado'. You could argue that we
> > > should mark the coverity issue as false-positive instead if you like.
> > > ---
> > >  target/xtensa/exc_helper.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> >
> > Acked-by: Max Filippov <jcmvbkbc@gmail.com>
>
> Thanks. I'll take it via target-arm since I'm doing a pullreq
> anyway, unless you'd prefer otherwise.

It's good, please go ahead.

-- 
Thanks.
-- Max