Re: [Qemu-devel] [PATCH v2 12/26] armv7m: check exception return consistency

[Peter Maydell <peter.maydell@linaro.org>]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Detect use of reserved exception return codes
> and return to thread mode from nested
> exception handler.
>
> Also check consistency between NVIC and CPU
> wrt. the active exception.
> ---
>  hw/intc/armv7m_nvic.c |  7 +++-
>  target-arm/cpu.h      |  2 +-
>  target-arm/helper.c   | 95 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 94 insertions(+), 10 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 619c320..7d261ae 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -396,7 +396,7 @@ void armv7m_nvic_acknowledge_irq(void *opaque)
>      assert(env->v7m.exception > 0); /* spurious exception? */
>  }
>
> -void armv7m_nvic_complete_irq(void *opaque, int irq)
> +bool armv7m_nvic_complete_irq(void *opaque, int irq)
>  {
>      NVICState *s = (NVICState *)opaque;
>      VecInfo *vec;
> @@ -406,12 +406,17 @@ void armv7m_nvic_complete_irq(void *opaque, int irq)
>
>      vec = &s->vectors[irq];
>
> +    if (!vec->active) {
> +        return true;
> +    }
> +
>      vec->active = 0;
>      vec->pending = vec->level;
>      assert(!vec->level || irq >= 16);
>
>      nvic_irq_update(s);
>      DPRINTF(0, "EOI %d\n", irq);
> +    return false;
>  }
>
>  /* Only called for external interrupt (vector>=16) */
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 4262efc..b98ef89 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1043,7 +1043,7 @@ void armv7m_nvic_set_pending(void *opaque, int irq);
>  bool armv7m_nvic_is_active(void *opaque, int irq);
>  int armv7m_nvic_get_active_prio(void *opaque);
>  void armv7m_nvic_acknowledge_irq(void *opaque);
> -void armv7m_nvic_complete_irq(void *opaque, int irq);
> +bool armv7m_nvic_complete_irq(void *opaque, int irq);

A brief comment here describing what the return value means would be nice.

>
>  /* Interface for defining coprocessor registers.
>   * Registers are defined in tables of arm_cp_reginfo structs
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b6ec761..f7e496d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5375,18 +5375,65 @@ static void switch_v7m_sp(CPUARMState *env, int process)
>
>  static void do_v7m_exception_exit(CPUARMState *env)
>  {
> +    unsigned ufault = 0;
>      uint32_t type;
>      uint32_t xpsr;
>
> -    type = env->regs[15];
> +    if (env->v7m.exception == 0) {
> +        hw_error("Return from exception w/o active exception.  Logic error.");

Should this just be an assert(), or can the guest provoke this?

> +    }
> +
>      if (env->v7m.exception != ARMV7M_EXCP_NMI) {
>          /* Auto-clear FAULTMASK on return from other than NMI */
>          env->daif &= ~PSTATE_F;
>      }
> -    if (env->v7m.exception != 0) {
> -        armv7m_nvic_complete_irq(env->nvic, env->v7m.exception);
> +
> +    if (armv7m_nvic_complete_irq(env->nvic, env->v7m.exception)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "Requesting return from exception "
> +                      "from inactive exception %d\n",
> +                      env->v7m.exception);
> +        ufault = 1;
> +    }
> +    env->v7m.exception = -42; /* spoil, will be unstacked below */

I don't really like this. If you're going to do it please at least
use a symbolic constant rather than hardcoding -42.

> +    env->v7m.exception_prio = armv7m_nvic_get_active_prio(env->nvic);
> +
> +    type = env->regs[15] & 0xf;
> +    /* QEMU seems to clear the LSB at some point. */
> +    type |= 1;

This is a rather vague comment. The LSB of regs[15] is always
clear because Thumb PCs are 2-aligned. We don't ever store the
"Thumb mode" bit in regs[15] (and the hardware doesn't either).

> +
> +    switch (type) {
> +    case 0x1: /* Return to Handler mode */
> +        if (env->v7m.exception_prio == 0x100) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "Requesting return from exception "
> +                          "to Handler mode not allowed at base level of "
> +                          "activation");
> +            ufault = 1;

What is this test? None of the listed integrity checks in section
B1.5.8 of the v7M ARM ARM or the ExceptionReturn pseudocode cehck
the priority of the currently executing exception.

> +        }
> +        break;
> +    case 0x9: /* Return to Thread mode w/ Main stack */
> +    case 0xd: /* Return to Thread mode w/ Process stack */
> +        if (env->v7m.exception_prio != 0x100) {
> +            /* Attempt to return to Thread mode
> +             * from nested handler while NONBASETHRDENA not set.
> +             */
> +            qemu_log_mask(LOG_GUEST_ERROR, "Nested exception return to %d w/"
> +                          " Thread mode while NONBASETHRDENA not set\n",
> +                          env->v7m.exception);

The error message and the comment talk about NONBASETHRDENA, but
the code isn't testing it. Also, you have nothing corresponding
to what the pseudocode NestedActivation variable is tracking
(ie is there a single active exception currently).

> +            ufault = 1;
> +        }
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "Exception return w/ reserved code"
> +                                       " %02x\n", (unsigned)type);
> +        ufault = 1;
>      }
>
> +    /* TODO? if ufault==1 ARM calls for entering exception handler
> +     * directly w/o popping stack.
> +     * We pop anyway since the active UsageFault will push on entry
> +     * which should happen before execution resumes?
> +     */

There is a distinction if SP is currently pointing at an invalid
address -- if you try to unstack and stack again then that will
fault and the guest can see the difference. This is a corner case
that I'm happy with us just marking as TODO for the moment though.
(Also I think there may be a difference if we have a pending NMI
at this point, but I haven't thought that through.)

> +
>      /* Switch to the target stack.  */
>      switch_v7m_sp(env, (type & 4) != 0);
>      /* Pop registers.  */
> @@ -5409,14 +5456,46 @@ static void do_v7m_exception_exit(CPUARMState *env)
>      }
>      xpsr = v7m_pop(env);
>      xpsr_write(env, xpsr, 0xfffffdff);
> +
> +    assert(env->v7m.exception!=-42);
> +
>      /* Undo stack alignment.  */
>      if (xpsr & 0x200)
>          env->regs[13] |= 4;
> -    /* ??? The exception return type specifies Thread/Handler mode.  However
> -       this is also implied by the xPSR value. Not sure what to do
> -       if there is a mismatch.  */
> -    /* ??? Likewise for mismatches between the CONTROL register and the stack
> -       pointer.  */
> +
> +    if (!ufault) {
> +        /* consistency check between NVIC and guest stack */
> +        if (env->v7m.exception == 0 && env->v7m.exception_prio != 0x100) {
> +            ufault = 1;
> +            qemu_log_mask(LOG_GUEST_ERROR, "Can't Unstacked to thread mode "
> +                          "with active exception\n");
> +            env->v7m.exception_prio = 0x100;
> +
> +        } else if (env->v7m.exception != 0 &&
> +                   !armv7m_nvic_is_active(env->nvic, env->v7m.exception))
> +        {
> +            ufault = 1;
> +            qemu_log_mask(LOG_GUEST_ERROR, "Unstacked exception %d is not "
> +                          "active\n", env->v7m.exception);
> +        } else  if (env->v7m.exception != 0
> +                    && env->v7m.exception_prio == 0x100) {
> +            hw_error("logic error at exception exit\n");
> +        }
> +        /* ARM calls for PushStack() here, which should happen
> +         * went we return with a pending exception

"when"

> +         */
> +    }
> +
> +    if (ufault) {
> +        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> +        env->v7m.cfsr |= 1<<18; /* INVPC */
> +    }
> +
> +    /* Ensure that priority is consistent.  Clear for Thread mode
> +     * and set for Handler mode
> +     */
> +    assert((env->v7m.exception == 0 && env->v7m.exception_prio > 0xff)
> +           || (env->v7m.exception != 0 && env->v7m.exception_prio <= 0xff));
>  }
>
>  static

thanks
-- PMM