[PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64

[PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64

[Peter Maydell]

Coverity reported that we don't set the _sifields union when queuing
the SIGTRAP for EXCP_DEBUG events on aarch64. This series fixes that
bug and a few others, and cleans up the way we queue fault signals
to be less error-prone.

The underlying cause of the bug is that when queueing a signal
the code must set the right field in the _sifields union depending
on the si_type that it passes to queue_signal(), and there's nothing
guarding against forgetting to do this. The "fill in a whole struct
and then call queue_signal()" code is also a bit longwinded. In the
real kernel, there is a function force_sig_fault() which does this
for the SI_FAULT signal type. The series provides a QEMU implementation
of this (which goes alongside the existing force_sig() that does this
for SI_KILL signals), and uses it in the arm and aarch64 code.
Because force_sig_fault() takes the address as an argument, it's
not possible for the caller to forget to fill it in.

Obviously we should also convert the other targets where they are
directly calling queue_signal() (there are other places that forget
to fill in the sifields union fields; I don't know why Coverity
hasn't spotted those). But I thought it better to send this out
for review of the new API and approach before spending time on
converting other targets. (In particular fixing places where
EXCP_DEBUG handling forgets to set the sifields address is a
pain, because in the real kernel different architectures might
either report the PC or else report a zero address here, so it
requires looking into the kernel sources to check which...)
Once all the archs are cleaned up we might be able to make
queue_signal() static so it's local to signal.c.

PS: there's probably a trivial conflict with my include-file-split
patchseries.

thanks
-- PMM

Peter Maydell (7):
  linux-user/aarch64: Set siginfo_t addr field for SIGTRAP signals
  linux-user/arm: Set siginfo_t addr field for SIGTRAP signals
  linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE
  linux-user: Zero out target_siginfo_t in force_sig()
  linux-user: Provide new force_sig_fault() function
  linux-user/arm: Use force_sig_fault()
  linux-user/aarch64: Use force_sig_fault()

 linux-user/signal-common.h    |  1 +
 linux-user/aarch64/cpu_loop.c | 33 +++++-------------
 linux-user/arm/cpu_loop.c     | 64 +++++++++++------------------------
 linux-user/signal.c           | 19 ++++++++++-
 4 files changed, 48 insertions(+), 69 deletions(-)

-- 
2.20.1

[PATCH for-6.2 1/7] linux-user/aarch64: Set siginfo_t addr field for SIGTRAP signals

[Peter Maydell]

When generating a TRAP_BRKPT SIGTRAP, set the siginfo_t addr field
to the PC where the breakpoint/singlestep trap occurred; this is
what the kernel does for this signal for this architecture.

Fixes: Coverity 1459154
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/aarch64/cpu_loop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index ee72a1c20f0..5d8675944d9 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -148,6 +148,7 @@ void cpu_loop(CPUARMState *env)
             info.si_signo = TARGET_SIGTRAP;
             info.si_errno = 0;
             info.si_code = TARGET_TRAP_BRKPT;
+            info._sifields._sigfault._addr = env->pc;
             queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
             break;
         case EXCP_SEMIHOST:
-- 
2.20.1

[PATCH for-6.2 2/7] linux-user/arm: Set siginfo_t addr field for SIGTRAP signals

[Peter Maydell]

When generating a TRAP_BRKPT SIGTRAP, set the siginfo_t addr field
to the PC where the breakpoint/singlestep trap occurred; this is
what the kernel does for this signal for this architecture.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/arm/cpu_loop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 69632d15be1..007752f5b74 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -453,6 +453,7 @@ void cpu_loop(CPUARMState *env)
             info.si_signo = TARGET_SIGTRAP;
             info.si_errno = 0;
             info.si_code = TARGET_TRAP_BRKPT;
+            info._sifields._sigfault._addr = env->regs[15];
             queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
             break;
         case EXCP_KERNEL_TRAP:
-- 
2.20.1

[PATCH for-6.2 3/7] linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE

[Peter Maydell]

In the Arm target code, when the fpa11 emulation code tells us we
need to send the guest a SIGFPE, we do this with queue_signal(), but
we are using the wrong si_type, and we aren't setting the _sifields
union members corresponding to either the si_type we are using or the
si_type we should be using.

As the existing comment notes, the kernel code for this calls the old
send_sig() function to deliver the signal.  This eventually results
in the kernel's signal handling code fabricating a siginfo_t with a
SI_KERNEL code and a zero pid and uid.  For QEMU this means we need
to use QEMU_SI_KILL.  We already have a function for that:
force_sig() sets up the whole target_siginfo_t the way we need it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/arm/cpu_loop.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 007752f5b74..44324976196 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -266,16 +266,13 @@ static bool emulate_arm_fpa11(CPUARMState *env, uint32_t opcode)
     ts->fpa.fpsr |= raise & ~enabled;
 
     if (raise & enabled) {
-        target_siginfo_t info = { };
-
         /*
          * The kernel's nwfpe emulator does not pass a real si_code.
-         * It merely uses send_sig(SIGFPE, current, 1).
+         * It merely uses send_sig(SIGFPE, current, 1), which results in
+         * __send_signal() filling out SI_KERNEL with pid and uid 0 (under
+         * the "SEND_SIG_PRIV" case). That's what our force_sig() does.
          */
-        info.si_signo = TARGET_SIGFPE;
-        info.si_code = TARGET_SI_KERNEL;
-
-        queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+        force_sig(TARGET_SIGFPE);
     } else {
         env->regs[15] += 4;
     }
-- 
2.20.1

[PATCH for-6.2 4/7] linux-user: Zero out target_siginfo_t in force_sig()

[Peter Maydell]

The target_siginfo_t we populate in force_sig() will eventually
get copied onto the target's stack. Zero it out so that any extra
padding in the sifields union is consistently zero when the guest
sees it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index a8faea6f090..fd3c6a3e60d 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -636,7 +636,7 @@ void force_sig(int sig)
 {
     CPUState *cpu = thread_cpu;
     CPUArchState *env = cpu->env_ptr;
-    target_siginfo_t info;
+    target_siginfo_t info = {};
 
     info.si_signo = sig;
     info.si_errno = 0;
-- 
2.20.1

[PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function

[Peter Maydell]

In many places in the linux-user code we need to queue a signal for
the guest using the QEMU_SI_FAULT si_type.  This requires that the
caller sets up and passes us a target_siginfo, including setting the
appropriate part of the _sifields union for the si_type. In a number
of places the code forgets to set the _sifields union field.

Provide a new force_sig_fault() function, which does the same thing
as the Linux kernel function of that name -- it takes the signal
number, the si_code value and the address to use in
_sifields._sigfault, and assembles the target_siginfo itself.  This
makes the callsites simpler and means it's harder to forget to pass
in an address value.

We follow force_sig() and the kernel's force_sig_fault() in not
requiring the caller to pass in the CPU pointer but always acting
on the CPU of the current thread.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal-common.h |  1 +
 linux-user/signal.c        | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/linux-user/signal-common.h b/linux-user/signal-common.h
index ea86328b289..536c7ac2c20 100644
--- a/linux-user/signal-common.h
+++ b/linux-user/signal-common.h
@@ -40,6 +40,7 @@ void tswap_siginfo(target_siginfo_t *tinfo,
 void set_sigmask(const sigset_t *set);
 void force_sig(int sig);
 void force_sigsegv(int oldsig);
+void force_sig_fault(int sig, int code, abi_ulong addr);
 #if defined(TARGET_ARCH_HAS_SETUP_FRAME)
 void setup_frame(int sig, struct target_sigaction *ka,
                  target_sigset_t *set, CPUArchState *env);
diff --git a/linux-user/signal.c b/linux-user/signal.c
index fd3c6a3e60d..5ea8e4584a7 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -646,6 +646,23 @@ void force_sig(int sig)
     queue_signal(env, info.si_signo, QEMU_SI_KILL, &info);
 }
 
+/*
+ * Force a synchronously taken QEMU_SI_FAULT signal. For QEMU the
+ * 'force' part is handled in process_pending_signals().
+ */
+void force_sig_fault(int sig, int code, abi_ulong addr)
+{
+    CPUState *cpu = thread_cpu;
+    CPUArchState *env = cpu->env_ptr;
+    target_siginfo_t info = {};
+
+    info.si_signo = sig;
+    info.si_errno = 0;
+    info.si_code = code;
+    info._sifields._sigfault._addr = addr;
+    queue_signal(env, sig, QEMU_SI_FAULT, &info);
+}
+
 /* Force a SIGSEGV if we couldn't write to memory trying to set
  * up the signal frame. oldsig is the signal we were trying to handle
  * at the point of failure.
-- 
2.20.1

[PATCH for-6.2 6/7] linux-user/arm: Use force_sig_fault()

[Peter Maydell]

Use the new force_sig_fault() function instead of setting up
a target_siginfo_t and calling queue_signal().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I threw in a comment confirming that the si_addr value for the "bad
SWI immediate" SIGILL really is different from the PC value reported
in the ucontext_t and resumed from if the handler returns, because it
looked like a bug to me when I was reading the code...
---
 linux-user/arm/cpu_loop.c | 54 ++++++++++++---------------------------
 1 file changed, 16 insertions(+), 38 deletions(-)

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 44324976196..d4b4f0c71fc 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -22,6 +22,7 @@
 #include "qemu.h"
 #include "elf.h"
 #include "cpu_loop-common.h"
+#include "signal-common.h"
 #include "semihosting/common-semi.h"
 
 #define get_user_code_u32(x, gaddr, env)                \
@@ -92,7 +93,6 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
 {
     uint64_t oldval, newval, val;
     uint32_t addr, cpsr;
-    target_siginfo_t info;
 
     /* Based on the 32 bit code in do_kernel_trap */
 
@@ -141,12 +141,9 @@ segv:
     end_exclusive();
     /* We get the PC of the entry address - which is as good as anything,
        on a real kernel what you get depends on which mode it uses. */
-    info.si_signo = TARGET_SIGSEGV;
-    info.si_errno = 0;
     /* XXX: check env->error_code */
-    info.si_code = TARGET_SEGV_MAPERR;
-    info._sifields._sigfault._addr = env->exception.vaddress;
-    queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+    force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MAPERR,
+                    env->exception.vaddress);
 }
 
 /* Handle a jump to the kernel code page.  */
@@ -284,8 +281,6 @@ void cpu_loop(CPUARMState *env)
     CPUState *cs = env_cpu(env);
     int trapnr;
     unsigned int n, insn;
-    target_siginfo_t info;
-    uint32_t addr;
     abi_ulong ret;
 
     for(;;) {
@@ -320,11 +315,8 @@ void cpu_loop(CPUARMState *env)
                     break;
                 }
 
-                info.si_signo = TARGET_SIGILL;
-                info.si_errno = 0;
-                info.si_code = TARGET_ILL_ILLOPN;
-                info._sifields._sigfault._addr = env->regs[15];
-                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+                force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPN,
+                                env->regs[15]);
             }
             break;
         case EXCP_SWI:
@@ -392,18 +384,14 @@ void cpu_loop(CPUARMState *env)
                              * Otherwise SIGILL. This includes any SWI with
                              * immediate not originally 0x9fxxxx, because
                              * of the earlier XOR.
+                             * Like the real kernel, we report the addr of the
+                             * SWI in the siginfo si_addr but leave the PC
+                             * pointing at the insn after the SWI.
                              */
-                            info.si_signo = TARGET_SIGILL;
-                            info.si_errno = 0;
-                            info.si_code = TARGET_ILL_ILLTRP;
-                            info._sifields._sigfault._addr = env->regs[15];
-                            if (env->thumb) {
-                                info._sifields._sigfault._addr -= 2;
-                            } else {
-                                info._sifields._sigfault._addr -= 4;
-                            }
-                            queue_signal(env, info.si_signo,
-                                         QEMU_SI_FAULT, &info);
+                            abi_ulong faultaddr = env->regs[15];
+                            faultaddr -= env->thumb ? 2 : 4;
+                            force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLTRP,
+                                            faultaddr);
                         }
                         break;
                     }
@@ -434,24 +422,14 @@ void cpu_loop(CPUARMState *env)
             break;
         case EXCP_PREFETCH_ABORT:
         case EXCP_DATA_ABORT:
-            addr = env->exception.vaddress;
-            {
-                info.si_signo = TARGET_SIGSEGV;
-                info.si_errno = 0;
-                /* XXX: check env->error_code */
-                info.si_code = TARGET_SEGV_MAPERR;
-                info._sifields._sigfault._addr = addr;
-                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
-            }
+            /* XXX: check env->error_code */
+            force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MAPERR,
+                            env->exception.vaddress);
             break;
         case EXCP_DEBUG:
         case EXCP_BKPT:
         excp_debug:
-            info.si_signo = TARGET_SIGTRAP;
-            info.si_errno = 0;
-            info.si_code = TARGET_TRAP_BRKPT;
-            info._sifields._sigfault._addr = env->regs[15];
-            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            force_sig_fault(TARGET_SIGTRAP, TARGET_TRAP_BRKPT, env->regs[15]);
             break;
         case EXCP_KERNEL_TRAP:
             if (do_kernel_trap(env))
-- 
2.20.1

[PATCH for-6.2 7/7] linux-user/aarch64: Use force_sig_fault()

[Peter Maydell]

Use the new force_sig_fault() function instead of setting up
a target_siginfo_t and calling queue_signal().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/aarch64/cpu_loop.c | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index 5d8675944d9..11e34cb1007 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -21,6 +21,7 @@
 #include "qemu-common.h"
 #include "qemu.h"
 #include "cpu_loop-common.h"
+#include "signal-common.h"
 #include "qemu/guest-random.h"
 #include "semihosting/common-semi.h"
 #include "target/arm/syndrome.h"
@@ -77,9 +78,8 @@
 void cpu_loop(CPUARMState *env)
 {
     CPUState *cs = env_cpu(env);
-    int trapnr, ec, fsc;
+    int trapnr, ec, fsc, si_code;
     abi_long ret;
-    target_siginfo_t info;
 
     for (;;) {
         cpu_exec_start(cs);
@@ -108,18 +108,10 @@ void cpu_loop(CPUARMState *env)
             /* just indicate that signals should be handled asap */
             break;
         case EXCP_UDEF:
-            info.si_signo = TARGET_SIGILL;
-            info.si_errno = 0;
-            info.si_code = TARGET_ILL_ILLOPN;
-            info._sifields._sigfault._addr = env->pc;
-            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPN, env->pc);
             break;
         case EXCP_PREFETCH_ABORT:
         case EXCP_DATA_ABORT:
-            info.si_signo = TARGET_SIGSEGV;
-            info.si_errno = 0;
-            info._sifields._sigfault._addr = env->exception.vaddress;
-
             /* We should only arrive here with EC in {DATAABORT, INSNABORT}. */
             ec = syn_get_ec(env->exception.syndrome);
             assert(ec == EC_DATAABORT || ec == EC_INSNABORT);
@@ -128,28 +120,24 @@ void cpu_loop(CPUARMState *env)
             fsc = extract32(env->exception.syndrome, 0, 6);
             switch (fsc) {
             case 0x04 ... 0x07: /* Translation fault, level {0-3} */
-                info.si_code = TARGET_SEGV_MAPERR;
+                si_code = TARGET_SEGV_MAPERR;
                 break;
             case 0x09 ... 0x0b: /* Access flag fault, level {1-3} */
             case 0x0d ... 0x0f: /* Permission fault, level {1-3} */
-                info.si_code = TARGET_SEGV_ACCERR;
+                si_code = TARGET_SEGV_ACCERR;
                 break;
             case 0x11: /* Synchronous Tag Check Fault */
-                info.si_code = TARGET_SEGV_MTESERR;
+                si_code = TARGET_SEGV_MTESERR;
                 break;
             default:
                 g_assert_not_reached();
             }
 
-            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            force_sig_fault(TARGET_SIGSEGV, si_code, env->exception.vaddress);
             break;
         case EXCP_DEBUG:
         case EXCP_BKPT:
-            info.si_signo = TARGET_SIGTRAP;
-            info.si_errno = 0;
-            info.si_code = TARGET_TRAP_BRKPT;
-            info._sifields._sigfault._addr = env->pc;
-            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            force_sig_fault(TARGET_SIGTRAP, TARGET_TRAP_BRKPT, env->pc);
             break;
         case EXCP_SEMIHOST:
             env->xregs[0] = do_common_semihosting(cs);
@@ -169,11 +157,7 @@ void cpu_loop(CPUARMState *env)
         /* Check for MTE asynchronous faults */
         if (unlikely(env->cp15.tfsr_el[0])) {
             env->cp15.tfsr_el[0] = 0;
-            info.si_signo = TARGET_SIGSEGV;
-            info.si_errno = 0;
-            info._sifields._sigfault._addr = 0;
-            info.si_code = TARGET_SEGV_MTEAERR;
-            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MTEAERR, 0);
         }
 
         process_pending_signals(env);
-- 
2.20.1

Re: [PATCH for-6.2 1/7] linux-user/aarch64: Set siginfo_t addr field for SIGTRAP signals

[Richard Henderson]

On 8/13/21 3:18 AM, Peter Maydell wrote:
> When generating a TRAP_BRKPT SIGTRAP, set the siginfo_t addr field
> to the PC where the breakpoint/singlestep trap occurred; this is
> what the kernel does for this signal for this architecture.
> 
> Fixes: Coverity 1459154
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   linux-user/aarch64/cpu_loop.c | 1 +
>   1 file changed, 1 insertion(+)

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

r~

Re: [PATCH for-6.2 2/7] linux-user/arm: Set siginfo_t addr field for SIGTRAP signals

[Richard Henderson]

On 8/13/21 3:18 AM, Peter Maydell wrote:
> When generating a TRAP_BRKPT SIGTRAP, set the siginfo_t addr field
> to the PC where the breakpoint/singlestep trap occurred; this is
> what the kernel does for this signal for this architecture.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   linux-user/arm/cpu_loop.c | 1 +
>   1 file changed, 1 insertion(+)

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

r~

Re: [PATCH for-6.2 3/7] linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE

[Richard Henderson]

On 8/13/21 3:18 AM, Peter Maydell wrote:
> In the Arm target code, when the fpa11 emulation code tells us we
> need to send the guest a SIGFPE, we do this with queue_signal(), but
> we are using the wrong si_type, and we aren't setting the _sifields
> union members corresponding to either the si_type we are using or the
> si_type we should be using.
> 
> As the existing comment notes, the kernel code for this calls the old
> send_sig() function to deliver the signal.  This eventually results
> in the kernel's signal handling code fabricating a siginfo_t with a
> SI_KERNEL code and a zero pid and uid.  For QEMU this means we need
> to use QEMU_SI_KILL.  We already have a function for that:
> force_sig() sets up the whole target_siginfo_t the way we need it.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   linux-user/arm/cpu_loop.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)

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

r~

Re: [PATCH for-6.2 4/7] linux-user: Zero out target_siginfo_t in force_sig()

[Richard Henderson]

On 8/13/21 3:18 AM, Peter Maydell wrote:
> The target_siginfo_t we populate in force_sig() will eventually
> get copied onto the target's stack. Zero it out so that any extra
> padding in the sifields union is consistently zero when the guest
> sees it.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   linux-user/signal.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

Re: [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function

[Richard Henderson]

On 8/13/21 3:18 AM, Peter Maydell wrote:
> +void force_sig_fault(int sig, int code, abi_ulong addr)

Better as abi_ptr?

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

r~

Re: [PATCH for-6.2 6/7] linux-user/arm: Use force_sig_fault()

[Richard Henderson]

On 8/13/21 3:18 AM, Peter Maydell wrote:
> Use the new force_sig_fault() function instead of setting up
> a target_siginfo_t and calling queue_signal().
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> I threw in a comment confirming that the si_addr value for the "bad
> SWI immediate" SIGILL really is different from the PC value reported
> in the ucontext_t and resumed from if the handler returns, because it
> looked like a bug to me when I was reading the code...
> ---
>   linux-user/arm/cpu_loop.c | 54 ++++++++++++---------------------------
>   1 file changed, 16 insertions(+), 38 deletions(-)

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

r~

Re: [PATCH for-6.2 7/7] linux-user/aarch64: Use force_sig_fault()

[Richard Henderson]

On 8/13/21 3:18 AM, Peter Maydell wrote:
> Use the new force_sig_fault() function instead of setting up
> a target_siginfo_t and calling queue_signal().
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   linux-user/aarch64/cpu_loop.c | 34 +++++++++-------------------------
>   1 file changed, 9 insertions(+), 25 deletions(-)

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

r~

Re: [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function

[Peter Maydell]

On Sun, 15 Aug 2021 at 21:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/13/21 3:18 AM, Peter Maydell wrote:
> > +void force_sig_fault(int sig, int code, abi_ulong addr)
>
> Better as abi_ptr?

I followed the same type used for 'addr' in the target_siginfo_t
struct definition.

-- PMM

Re: [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function

[Richard Henderson]

On 8/15/21 11:03 PM, Peter Maydell wrote:
> On Sun, 15 Aug 2021 at 21:10, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 8/13/21 3:18 AM, Peter Maydell wrote:
>>> +void force_sig_fault(int sig, int code, abi_ulong addr)
>>
>> Better as abi_ptr?
> 
> I followed the same type used for 'addr' in the target_siginfo_t
> struct definition.

Fair enough.

r~

Re: [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64

[Laurent Vivier]

Le 13/08/2021 à 15:18, Peter Maydell a écrit :
> Coverity reported that we don't set the _sifields union when queuing
> the SIGTRAP for EXCP_DEBUG events on aarch64. This series fixes that
> bug and a few others, and cleans up the way we queue fault signals
> to be less error-prone.
> 
> The underlying cause of the bug is that when queueing a signal
> the code must set the right field in the _sifields union depending
> on the si_type that it passes to queue_signal(), and there's nothing
> guarding against forgetting to do this. The "fill in a whole struct
> and then call queue_signal()" code is also a bit longwinded. In the
> real kernel, there is a function force_sig_fault() which does this
> for the SI_FAULT signal type. The series provides a QEMU implementation
> of this (which goes alongside the existing force_sig() that does this
> for SI_KILL signals), and uses it in the arm and aarch64 code.
> Because force_sig_fault() takes the address as an argument, it's
> not possible for the caller to forget to fill it in.
> 
> Obviously we should also convert the other targets where they are
> directly calling queue_signal() (there are other places that forget
> to fill in the sifields union fields; I don't know why Coverity
> hasn't spotted those). But I thought it better to send this out
> for review of the new API and approach before spending time on
> converting other targets. (In particular fixing places where
> EXCP_DEBUG handling forgets to set the sifields address is a
> pain, because in the real kernel different architectures might
> either report the PC or else report a zero address here, so it
> requires looking into the kernel sources to check which...)
> Once all the archs are cleaned up we might be able to make
> queue_signal() static so it's local to signal.c.
> 
> PS: there's probably a trivial conflict with my include-file-split
> patchseries.
> 
> thanks
> -- PMM
> 
> Peter Maydell (7):
>   linux-user/aarch64: Set siginfo_t addr field for SIGTRAP signals
>   linux-user/arm: Set siginfo_t addr field for SIGTRAP signals
>   linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE
>   linux-user: Zero out target_siginfo_t in force_sig()
>   linux-user: Provide new force_sig_fault() function
>   linux-user/arm: Use force_sig_fault()
>   linux-user/aarch64: Use force_sig_fault()
> 
>  linux-user/signal-common.h    |  1 +
>  linux-user/aarch64/cpu_loop.c | 33 +++++-------------
>  linux-user/arm/cpu_loop.c     | 64 +++++++++++------------------------
>  linux-user/signal.c           | 19 ++++++++++-
>  4 files changed, 48 insertions(+), 69 deletions(-)
> 

Applied to my linux-user-for-6.2 branch.

Thanks,
Laurent