[Qemu-devel] [PATCH for-4.1?] target/arm: Deliver BKPT/BRK exceptions to correct exception level

[Qemu-devel] [PATCH for-4.1?] target/arm: Deliver BKPT/BRK exceptions to correct exception level

[Peter Maydell]

Most Arm architectural debug exceptions (eg watchpoints) are ignored
if the configured "debug exception level" is below the current
exception level (so for example EL1 can't arrange to get debug exceptions
for EL2 execution). Exceptions generated by the BRK or BPKT instructions
are a special case -- they must always cause an exception, so if
we're executing above the debug exception level then we
must take them to the current exception level.

This fixes a bug where executing BRK at EL2 could result in an
exception being taken at EL1 (which is strictly forbidden by the
architecture).

Fixes: https://bugs.launchpad.net/qemu/+bug/1838277
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
At this point in the release cycle I'm not sure we should put this
into 4.1 -- it is definitely a bug but it's not a regression as
we've been wrong like this for multiple releases, pretty much
since we put in the debug handling code I suspect.

 target/arm/op_helper.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 1ab91f915e4..5e1625a1c8a 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -370,6 +370,9 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp,
  */
 void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
 {
+    int debug_el = arm_debug_target_el(env);
+    int cur_el = arm_current_el(env);
+
     /* FSR will only be used if the debug target EL is AArch32. */
     env->exception.fsr = arm_debug_exception_fsr(env);
     /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing
@@ -377,7 +380,18 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
      * exception/security level.
      */
     env->exception.vaddress = 0;
-    raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
+    /*
+     * Other kinds of architectural debug exception are ignored if
+     * they target an exception level below the current one (in QEMU
+     * this is checked by arm_generate_debug_exceptions()). Breakpoint
+     * instructions are special because they always generate an exception
+     * to somewhere: if they can't go to the configured debug exception
+     * level they are taken to the current exception level.
+     */
+    if (debug_el < cur_el) {
+        debug_el = cur_el;
+    }
+    raise_exception(env, EXCP_BKPT, syndrome, debug_el);
 }
 
 uint32_t HELPER(cpsr_read)(CPUARMState *env)
-- 
2.20.1

Re: [Qemu-devel] [PATCH for-4.1?] target/arm: Deliver BKPT/BRK exceptions to correct exception level

[Richard Henderson]

On 7/30/19 6:25 AM, Peter Maydell wrote:
> Most Arm architectural debug exceptions (eg watchpoints) are ignored
> if the configured "debug exception level" is below the current
> exception level (so for example EL1 can't arrange to get debug exceptions
> for EL2 execution). Exceptions generated by the BRK or BPKT instructions
> are a special case -- they must always cause an exception, so if
> we're executing above the debug exception level then we
> must take them to the current exception level.
> 
> This fixes a bug where executing BRK at EL2 could result in an
> exception being taken at EL1 (which is strictly forbidden by the
> architecture).
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1838277
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> At this point in the release cycle I'm not sure we should put this
> into 4.1 -- it is definitely a bug but it's not a regression as
> we've been wrong like this for multiple releases, pretty much
> since we put in the debug handling code I suspect.

I'd lean to putting it in, fwiw.


r~

Re: [Qemu-devel] [PATCH for-4.1?] target/arm: Deliver BKPT/BRK exceptions to correct exception level

[Philippe Mathieu-Daudé]

On 7/30/19 3:25 PM, Peter Maydell wrote:
> Most Arm architectural debug exceptions (eg watchpoints) are ignored
> if the configured "debug exception level" is below the current
> exception level (so for example EL1 can't arrange to get debug exceptions
> for EL2 execution). Exceptions generated by the BRK or BPKT instructions
> are a special case -- they must always cause an exception, so if
> we're executing above the debug exception level then we
> must take them to the current exception level.
> 
> This fixes a bug where executing BRK at EL2 could result in an
> exception being taken at EL1 (which is strictly forbidden by the
> architecture).
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1838277
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> At this point in the release cycle I'm not sure we should put this
> into 4.1 -- it is definitely a bug but it's not a regression as
> we've been wrong like this for multiple releases, pretty much
> since we put in the debug handling code I suspect.

The fix is quite trivial, and the user reported using a release, so
having it in the next release would be nice.
Or as usual, wait for 'last-minute-bugfix-that-postpone-another-rc' and
squeeze this fix in.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>  target/arm/op_helper.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 1ab91f915e4..5e1625a1c8a 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -370,6 +370,9 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp,
>   */
>  void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
>  {
> +    int debug_el = arm_debug_target_el(env);
> +    int cur_el = arm_current_el(env);
> +
>      /* FSR will only be used if the debug target EL is AArch32. */
>      env->exception.fsr = arm_debug_exception_fsr(env);
>      /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing
> @@ -377,7 +380,18 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
>       * exception/security level.
>       */
>      env->exception.vaddress = 0;
> -    raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
> +    /*
> +     * Other kinds of architectural debug exception are ignored if
> +     * they target an exception level below the current one (in QEMU
> +     * this is checked by arm_generate_debug_exceptions()). Breakpoint
> +     * instructions are special because they always generate an exception
> +     * to somewhere: if they can't go to the configured debug exception
> +     * level they are taken to the current exception level.
> +     */
> +    if (debug_el < cur_el) {
> +        debug_el = cur_el;
> +    }
> +    raise_exception(env, EXCP_BKPT, syndrome, debug_el);
>  }
>  
>  uint32_t HELPER(cpsr_read)(CPUARMState *env)
> 

Re: [Qemu-devel] [PATCH for-4.1?] target/arm: Deliver BKPT/BRK exceptions to correct exception level

[Peter Maydell]

On Tue, 30 Jul 2019 at 16:31, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 7/30/19 3:25 PM, Peter Maydell wrote:
> > Most Arm architectural debug exceptions (eg watchpoints) are ignored
> > if the configured "debug exception level" is below the current
> > exception level (so for example EL1 can't arrange to get debug exceptions
> > for EL2 execution). Exceptions generated by the BRK or BPKT instructions
> > are a special case -- they must always cause an exception, so if
> > we're executing above the debug exception level then we
> > must take them to the current exception level.
> >
> > This fixes a bug where executing BRK at EL2 could result in an
> > exception being taken at EL1 (which is strictly forbidden by the
> > architecture).
> >
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1838277
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > At this point in the release cycle I'm not sure we should put this
> > into 4.1 -- it is definitely a bug but it's not a regression as
> > we've been wrong like this for multiple releases, pretty much
> > since we put in the debug handling code I suspect.
>
> The fix is quite trivial, and the user reported using a release, so
> having it in the next release would be nice.
> Or as usual, wait for 'last-minute-bugfix-that-postpone-another-rc' and
> squeeze this fix in.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

OK, people seem to think it's worth putting in, so I've applied
it to master.

thanks
-- PMM