[PATCH 0/3] linux-user/aarch64: Add ESR signal frame record

[PATCH 0/3] linux-user/aarch64: Add ESR signal frame record

[Richard Henderson]

We use this ourselves in qemu for SIGSEGV handling, and the kernel
now adds one for the FPAC fault.


r~


Richard Henderson (3):
  linux-user/aarch64: Add ESR signal frame for SIGSEGV, SIGBUS
  linux-user/aarch64: Fix normal SIGILL si_code
  linux-user/aarch64: Add ESR signal frame for PACFAIL

 linux-user/aarch64/cpu_loop.c |  7 ++++-
 linux-user/aarch64/signal.c   | 54 ++++++++++++++++++++++++++++++++++-
 tests/tcg/aarch64/pauth-2.c   | 25 +++++++++++++++-
 3 files changed, 83 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/3] linux-user/aarch64: Add ESR signal frame for SIGSEGV, SIGBUS

[Richard Henderson]

These are all synchronous exceptions for which the kernel
passes on ESR to the user signal handler.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/aarch64/signal.c | 48 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
index b265cfd470..b2280fa9e3 100644
--- a/linux-user/aarch64/signal.c
+++ b/linux-user/aarch64/signal.c
@@ -21,6 +21,7 @@
 #include "user-internals.h"
 #include "signal-common.h"
 #include "linux-user/trace.h"
+#include "target/arm/syndrome.h"
 
 struct target_sigcontext {
     uint64_t fault_address;
@@ -64,6 +65,13 @@ struct target_fpsimd_context {
     uint64_t vregs[32 * 2]; /* really uint128_t vregs[32] */
 };
 
+#define TARGET_ESR_MAGIC    0x45535201
+
+struct target_esr_context {
+    struct target_aarch64_ctx head;
+    uint64_t esr;
+};
+
 #define TARGET_EXTRA_MAGIC  0x45585401
 
 struct target_extra_context {
@@ -191,6 +199,14 @@ static void target_setup_end_record(struct target_aarch64_ctx *end)
     __put_user(0, &end->size);
 }
 
+static void target_setup_esr_record(struct target_esr_context *esr,
+                                    CPUARMState *env)
+{
+    __put_user(TARGET_ESR_MAGIC, &esr->head.magic);
+    __put_user(sizeof(struct target_esr_context), &esr->head.size);
+    __put_user(env->exception.syndrome, &esr->esr);
+}
+
 static void target_setup_sve_record(struct target_sve_context *sve,
                                     CPUARMState *env, int size)
 {
@@ -443,6 +459,10 @@ static int target_restore_sigframe(CPUARMState *env,
             fpsimd = (struct target_fpsimd_context *)ctx;
             break;
 
+        case TARGET_ESR_MAGIC:
+            /* ignore */
+            break;
+
         case TARGET_SVE_MAGIC:
             if (sve || size < sizeof(struct target_sve_context)) {
                 goto err;
@@ -558,6 +578,23 @@ static int alloc_sigframe_space(int this_size, target_sigframe_layout *l)
     return this_loc;
 }
 
+static bool need_save_esr(target_siginfo_t *info, CPUARMState *env)
+{
+    int sig = info->si_signo;
+    int type = info->si_code >> 16;
+
+    if (type != QEMU_SI_FAULT) {
+        return false;
+    }
+
+    /* See arch/arm64/mm/fault.c, set_thread_esr. */
+    if (sig == TARGET_SIGSEGV || sig == TARGET_SIGBUS) {
+        return true;
+    }
+
+    return false;
+}
+
 static void target_setup_frame(int usig, struct target_sigaction *ka,
                                target_siginfo_t *info, target_sigset_t *set,
                                CPUARMState *env)
@@ -567,7 +604,7 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
         .total_size = offsetof(struct target_rt_sigframe,
                                uc.tuc_mcontext.__reserved),
     };
-    int fpsimd_ofs, fr_ofs, sve_ofs = 0, za_ofs = 0;
+    int fpsimd_ofs, fr_ofs, esr_ofs = 0, sve_ofs = 0, za_ofs = 0;
     int sve_size = 0, za_size = 0;
     struct target_rt_sigframe *frame;
     struct target_rt_frame_record *fr;
@@ -577,6 +614,12 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
     fpsimd_ofs = alloc_sigframe_space(sizeof(struct target_fpsimd_context),
                                       &layout);
 
+    /* ESR state needs saving only for certain signals. */
+    if (need_save_esr(info, env)) {
+        esr_ofs = alloc_sigframe_space(sizeof(struct target_esr_context),
+                                       &layout);
+    }
+
     /* SVE state needs saving only if it exists.  */
     if (cpu_isar_feature(aa64_sve, env_archcpu(env)) ||
         cpu_isar_feature(aa64_sme, env_archcpu(env))) {
@@ -637,6 +680,9 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
                                   layout.extra_size);
         target_setup_end_record((void *)frame + layout.extra_end_ofs);
     }
+    if (esr_ofs) {
+        target_setup_esr_record((void *)frame + esr_ofs, env);
+    }
     if (sve_ofs) {
         target_setup_sve_record((void *)frame + sve_ofs, env, sve_size);
     }
-- 
2.34.1

Re: [PATCH 1/3] linux-user/aarch64: Add ESR signal frame for SIGSEGV, SIGBUS

[Peter Maydell]

On Tue, 22 Aug 2023 at 18:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> These are all synchronous exceptions for which the kernel
> passes on ESR to the user signal handler.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> @@ -191,6 +199,14 @@ static void target_setup_end_record(struct target_aarch64_ctx *end)
>      __put_user(0, &end->size);
>  }
>
> +static void target_setup_esr_record(struct target_esr_context *esr,
> +                                    CPUARMState *env)
> +{
> +    __put_user(TARGET_ESR_MAGIC, &esr->head.magic);
> +    __put_user(sizeof(struct target_esr_context), &esr->head.size);
> +    __put_user(env->exception.syndrome, &esr->esr);
> +}
> +
>  static void target_setup_sve_record(struct target_sve_context *sve,
>                                      CPUARMState *env, int size)
>  {
> @@ -443,6 +459,10 @@ static int target_restore_sigframe(CPUARMState *env,
>              fpsimd = (struct target_fpsimd_context *)ctx;
>              break;
>
> +        case TARGET_ESR_MAGIC:
> +            /* ignore */
> +            break;
> +
>          case TARGET_SVE_MAGIC:
>              if (sve || size < sizeof(struct target_sve_context)) {
>                  goto err;
> @@ -558,6 +578,23 @@ static int alloc_sigframe_space(int this_size, target_sigframe_layout *l)
>      return this_loc;
>  }
>
> +static bool need_save_esr(target_siginfo_t *info, CPUARMState *env)
> +{
> +    int sig = info->si_signo;
> +    int type = info->si_code >> 16;
> +
> +    if (type != QEMU_SI_FAULT) {
> +        return false;
> +    }
> +
> +    /* See arch/arm64/mm/fault.c, set_thread_esr. */
> +    if (sig == TARGET_SIGSEGV || sig == TARGET_SIGBUS) {
> +        return true;
> +    }

It's possible to get here without env->exception.syndrome
being set correctly, I think, if we take a host
SIGSEGV or SIGBUS and host_signal_handler() calls either
cpu_loop_exit_sigsegv() or cpu_loop_exit_sigbus(). Can also
happen for other places that call one of those two functions,
like allocation_tag_mem(). At least, I can't see where we
would be setting syndrome in that code path.

> +
> +    return false;
> +}

Maybe we should do the "sanitize ESR for fault addresses in
the upper half of guest address space" logic that the kernel
set_thread_esr() does?

thanks
-- PMM

Re: [PATCH 1/3] linux-user/aarch64: Add ESR signal frame for SIGSEGV, SIGBUS

[Richard Henderson]

On 8/29/23 07:35, Peter Maydell wrote:
>> +    /* See arch/arm64/mm/fault.c, set_thread_esr. */
>> +    if (sig == TARGET_SIGSEGV || sig == TARGET_SIGBUS) {
>> +        return true;
>> +    }
> 
> It's possible to get here without env->exception.syndrome
> being set correctly, I think, if we take a host
> SIGSEGV or SIGBUS and host_signal_handler() calls either
> cpu_loop_exit_sigsegv() or cpu_loop_exit_sigbus(). Can also
> happen for other places that call one of those two functions,
> like allocation_tag_mem(). At least, I can't see where we
> would be setting syndrome in that code path.

cpu_loop_exit_sig* go through arm_cpu_record_sigsegv and arm_cpu_record_sigbus, which use 
the normal fault processing paths to populate FAR_EL1 and ESR_EL1.

> Maybe we should do the "sanitize ESR for fault addresses in
> the upper half of guest address space" logic that the kernel
> set_thread_esr() does?

I guess we could, though I'm not sure how such an address could occur.


r~

[PATCH 2/3] linux-user/aarch64: Fix normal SIGILL si_code

[Richard Henderson]

Most illegal instructions use ILL_ILLOPC.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/aarch64/cpu_loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index 2e2f7cf218..22c9789326 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -110,7 +110,7 @@ void cpu_loop(CPUARMState *env)
             /* just indicate that signals should be handled asap */
             break;
         case EXCP_UDEF:
-            force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPN, env->pc);
+            force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPC, env->pc);
             break;
         case EXCP_PREFETCH_ABORT:
         case EXCP_DATA_ABORT:
-- 
2.34.1

Re: [PATCH 2/3] linux-user/aarch64: Fix normal SIGILL si_code

[Peter Maydell]

On Tue, 22 Aug 2023 at 18:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Most illegal instructions use ILL_ILLOPC.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/aarch64/cpu_loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
> index 2e2f7cf218..22c9789326 100644
> --- a/linux-user/aarch64/cpu_loop.c
> +++ b/linux-user/aarch64/cpu_loop.c
> @@ -110,7 +110,7 @@ void cpu_loop(CPUARMState *env)
>              /* just indicate that signals should be handled asap */
>              break;
>          case EXCP_UDEF:
> -            force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPN, env->pc);
> +            force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPC, env->pc);
>              break;
>          case EXCP_PREFETCH_ABORT:
>          case EXCP_DATA_ABORT:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 3/3] linux-user/aarch64: Add ESR signal frame for PACFAIL

[Richard Henderson]

The PACFAIL fault uses ILL_ILLOPN and includes ESR.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/aarch64/cpu_loop.c |  7 ++++++-
 linux-user/aarch64/signal.c   |  6 ++++++
 tests/tcg/aarch64/pauth-2.c   | 25 ++++++++++++++++++++++++-
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index 22c9789326..5af17e8724 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -110,7 +110,12 @@ void cpu_loop(CPUARMState *env)
             /* just indicate that signals should be handled asap */
             break;
         case EXCP_UDEF:
-            force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPC, env->pc);
+            /* See kernel's do_el0_fpac, and our need_save_esr(). */
+            if (syn_get_ec(env->exception.syndrome) == EC_PACFAIL) {
+                force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPN, env->pc);
+            } else {
+                force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPC, env->pc);
+            }
             break;
         case EXCP_PREFETCH_ABORT:
         case EXCP_DATA_ABORT:
diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
index b2280fa9e3..bcdd796cc2 100644
--- a/linux-user/aarch64/signal.c
+++ b/linux-user/aarch64/signal.c
@@ -582,6 +582,7 @@ static bool need_save_esr(target_siginfo_t *info, CPUARMState *env)
 {
     int sig = info->si_signo;
     int type = info->si_code >> 16;
+    int code = info->si_code & 0xffff;
 
     if (type != QEMU_SI_FAULT) {
         return false;
@@ -592,6 +593,11 @@ static bool need_save_esr(target_siginfo_t *info, CPUARMState *env)
         return true;
     }
 
+    /* See arch/arm64/kernel/traps.c, do_el0_fpac, and our cpu_loop(). */
+    if (sig == TARGET_SIGILL && code == TARGET_ILL_ILLOPN) {
+        return true;
+    }
+
     return false;
 }
 
diff --git a/tests/tcg/aarch64/pauth-2.c b/tests/tcg/aarch64/pauth-2.c
index d498d7dd8b..62b39af3d0 100644
--- a/tests/tcg/aarch64/pauth-2.c
+++ b/tests/tcg/aarch64/pauth-2.c
@@ -4,14 +4,37 @@
 #include <assert.h>
 #include <sys/auxv.h>
 
+static inline struct _aarch64_ctx *first_ctx(ucontext_t *uc)
+{
+    return (struct _aarch64_ctx *)&uc->uc_mcontext.__reserved;
+}
+
+static inline struct _aarch64_ctx *next_ctx(struct _aarch64_ctx *hdr)
+{
+    return (struct _aarch64_ctx *)((char *)hdr + hdr->size);
+}
+
 static void sigill(int sig, siginfo_t *info, void *vuc)
 {
     ucontext_t *uc = vuc;
-    uint64_t test;
+    struct _aarch64_ctx *hdr;
+    struct esr_context *ec;
+    uint64_t test, esr;
 
     /* There is only one insn below that is allowed to fault. */
     asm volatile("adr %0, auth2_insn" : "=r"(test));
     assert(test == uc->uc_mcontext.pc);
+
+    /* Find the esr_context. */
+    for (hdr = first_ctx(uc); hdr->magic != ESR_MAGIC; hdr = next_ctx(hdr)) {
+        assert(hdr->magic != 0);
+    }
+
+    ec = (struct esr_context *)hdr;
+    esr = ec->esr;
+
+    assert((esr >> 26) == 0x1c); /* EC_PACFAIL */
+    assert((esr & 3) == 2);      /* AUTDA: data=1 key=0 */
     exit(0);
 }
 
-- 
2.34.1

Re: [PATCH 3/3] linux-user/aarch64: Add ESR signal frame for PACFAIL

[Peter Maydell]

On Tue, 22 Aug 2023 at 18:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The PACFAIL fault uses ILL_ILLOPN and includes ESR.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/aarch64/cpu_loop.c |  7 ++++++-
>  linux-user/aarch64/signal.c   |  6 ++++++
>  tests/tcg/aarch64/pauth-2.c   | 25 ++++++++++++++++++++++++-
>  3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
> index 22c9789326..5af17e8724 100644
> --- a/linux-user/aarch64/cpu_loop.c
> +++ b/linux-user/aarch64/cpu_loop.c
> @@ -110,7 +110,12 @@ void cpu_loop(CPUARMState *env)
>              /* just indicate that signals should be handled asap */
>              break;
>          case EXCP_UDEF:
> -            force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPC, env->pc);
> +            /* See kernel's do_el0_fpac, and our need_save_esr(). */
> +            if (syn_get_ec(env->exception.syndrome) == EC_PACFAIL) {
> +                force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPN, env->pc);
> +            } else {
> +                force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPC, env->pc);
> +            }
>              break;
>          case EXCP_PREFETCH_ABORT:
>          case EXCP_DATA_ABORT:
> diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
> index b2280fa9e3..bcdd796cc2 100644
> --- a/linux-user/aarch64/signal.c
> +++ b/linux-user/aarch64/signal.c
> @@ -582,6 +582,7 @@ static bool need_save_esr(target_siginfo_t *info, CPUARMState *env)
>  {
>      int sig = info->si_signo;
>      int type = info->si_code >> 16;
> +    int code = info->si_code & 0xffff;
>
>      if (type != QEMU_SI_FAULT) {
>          return false;
> @@ -592,6 +593,11 @@ static bool need_save_esr(target_siginfo_t *info, CPUARMState *env)
>          return true;
>      }
>
> +    /* See arch/arm64/kernel/traps.c, do_el0_fpac, and our cpu_loop(). */
> +    if (sig == TARGET_SIGILL && code == TARGET_ILL_ILLOPN) {
> +        return true;
> +    }

This works, but we'll need to do something else if the kernel adds
some other fault later that is reported as ILLOPN but without
an ESR record...

> +
>      return false;
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/3] linux-user/aarch64: Add ESR signal frame for PACFAIL

[Richard Henderson]

On 8/29/23 07:46, Peter Maydell wrote:
>> +    /* See arch/arm64/kernel/traps.c, do_el0_fpac, and our cpu_loop(). */
>> +    if (sig == TARGET_SIGILL && code == TARGET_ILL_ILLOPN) {
>> +        return true;
>> +    }
> 
> This works, but we'll need to do something else if the kernel adds
> some other fault later that is reported as ILLOPN but without
> an ESR record...

Yes.  I'm not happy about the separation in logic, but I can't think of a better way at 
present.


r~