[PATCH 0/2] SIGSEGV fixes

[PATCH 0/2] SIGSEGV fixes

[Taylor Simpson]

The Hexagon target was silently failing the SIGSEGV test because
the signal handler was not called.

Patch 1/2 fixes the Hexagon target
Patch 2/2 adds a check that the signal handler is called

Taylor Simpson (2):
  Hexagon (target/hexagon) do probe_write in HELPER(commit_store)
  linux-test (tests/tcg/multiarch/linux-test.c) add check

 target/hexagon/op_helper.c       | 23 ++++++++++++++---------
 tests/tcg/multiarch/linux-test.c |  7 +++++++
 2 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH 1/2] Hexagon (target/hexagon) do probe_write in HELPER(commit_store)

[Taylor Simpson]

Check that access is OK before doing put_user_*

Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 target/hexagon/op_helper.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 4595559..d7f53a2 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -140,22 +140,27 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env, int slot, int check)
 
 void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
 {
-    switch (env->mem_log_stores[slot_num].width) {
+    uint8_t width = env->mem_log_stores[slot_num].width;
+    target_ulong va = env->mem_log_stores[slot_num].va;
+
+#ifdef CONFIG_USER_ONLY
+    g_assert(width == 1 || width == 2 || width == 4 || width == 8);
+    /* We perform this check elsewhere in system mode */
+    probe_write(env, va, width, MMU_USER_IDX, 0);
+#endif
+
+    switch (width) {
     case 1:
-        put_user_u8(env->mem_log_stores[slot_num].data32,
-                    env->mem_log_stores[slot_num].va);
+        put_user_u8(env->mem_log_stores[slot_num].data32, va);
         break;
     case 2:
-        put_user_u16(env->mem_log_stores[slot_num].data32,
-                     env->mem_log_stores[slot_num].va);
+        put_user_u16(env->mem_log_stores[slot_num].data32, va);
         break;
     case 4:
-        put_user_u32(env->mem_log_stores[slot_num].data32,
-                     env->mem_log_stores[slot_num].va);
+        put_user_u32(env->mem_log_stores[slot_num].data32, va);
         break;
     case 8:
-        put_user_u64(env->mem_log_stores[slot_num].data64,
-                     env->mem_log_stores[slot_num].va);
+        put_user_u64(env->mem_log_stores[slot_num].data64, va);
         break;
     default:
         g_assert_not_reached();
-- 
2.7.4

[PATCH 2/2] linux-test (tests/tcg/multiarch/linux-test.c) add check

[Taylor Simpson]

Add a check that the SIGSEGV handler is called

Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 tests/tcg/multiarch/linux-test.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
index c8c6aed..cb845c9 100644
--- a/tests/tcg/multiarch/linux-test.c
+++ b/tests/tcg/multiarch/linux-test.c
@@ -439,10 +439,13 @@ static void sig_alarm(int sig)
     alarm_count++;
 }
 
+static int sig_segv_called;
+
 static void sig_segv(int sig, siginfo_t *info, void *puc)
 {
     if (sig != SIGSEGV)
         error("signal");
+    sig_segv_called = 1;
     longjmp(jmp_env, 1);
 }
 
@@ -492,6 +495,10 @@ static void test_signal(void)
         *(volatile uint8_t *)0 = 0;
     }
 
+    if (sig_segv_called == 0) {
+        error("SIGSEGV handler not called");
+    }
+
     act.sa_handler = SIG_DFL;
     sigemptyset(&act.sa_mask);
     act.sa_flags = 0;
-- 
2.7.4

Re: [PATCH 1/2] Hexagon (target/hexagon) do probe_write in HELPER(commit_store)

[Richard Henderson]

On 7/13/21 12:46 PM, Taylor Simpson wrote:
>   void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
>   {
> -    switch (env->mem_log_stores[slot_num].width) {
> +    uint8_t width = env->mem_log_stores[slot_num].width;
> +    target_ulong va = env->mem_log_stores[slot_num].va;
> +
> +#ifdef CONFIG_USER_ONLY
> +    g_assert(width == 1 || width == 2 || width == 4 || width == 8);
> +    /* We perform this check elsewhere in system mode */
> +    probe_write(env, va, width, MMU_USER_IDX, 0);
> +#endif
> +
> +    switch (width) {
>       case 1:
> -        put_user_u8(env->mem_log_stores[slot_num].data32,
> -                    env->mem_log_stores[slot_num].va);
> +        put_user_u8(env->mem_log_stores[slot_num].data32, va);

The primary problem here is that put_user_* is the wrong set of functions to use.  You 
should have been using exec/cpu_ldst.h, in particular cpu_ld*_data_ra and cpu_st*_data_ra.


r~

Re: [PATCH 2/2] linux-test (tests/tcg/multiarch/linux-test.c) add check

[Richard Henderson]

On 7/13/21 12:46 PM, Taylor Simpson wrote:
> Add a check that the SIGSEGV handler is called
> 
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
>   tests/tcg/multiarch/linux-test.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
> index c8c6aed..cb845c9 100644
> --- a/tests/tcg/multiarch/linux-test.c
> +++ b/tests/tcg/multiarch/linux-test.c
> @@ -439,10 +439,13 @@ static void sig_alarm(int sig)
>       alarm_count++;
>   }
>   
> +static int sig_segv_called;
> +
>   static void sig_segv(int sig, siginfo_t *info, void *puc)
>   {
>       if (sig != SIGSEGV)
>           error("signal");
> +    sig_segv_called = 1;

Either bool or a count.

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

r~

>       longjmp(jmp_env, 1);
>   }
>   
> @@ -492,6 +495,10 @@ static void test_signal(void)
>           *(volatile uint8_t *)0 = 0;
>       }
>   
> +    if (sig_segv_called == 0) {
> +        error("SIGSEGV handler not called");
> +    }
> +
>       act.sa_handler = SIG_DFL;
>       sigemptyset(&act.sa_mask);
>       act.sa_flags = 0;
>