[Qemu-devel] [RESEND PATCH] target-arm/abi32: check for segfault in do_kernel_trap

[Qemu-devel] [RESEND PATCH] target-arm/abi32: check for segfault in do_kernel_trap

[Seraphime Kirkovski]

Currently, the cmpxchg implementation tests whether the destination address
is readable:
  - if it is, we read the value and continue with the comparison
  - if isn't, i.e. access to addr would segfault, we assume that src != dest
    rather than queuing a SIGSEGV.

The same problem exists in the case where src == dest: the code doesn't
check whether put_user_u32 succeeds.

This fixes both problems by sending a SIGSEGV when the destination address
is inaccessible.

Signed-off-by: Seraphime Kirkovski <kirkseraph@gmail.com>
---
 linux-user/main.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 75b199f..376b0c3 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -503,6 +503,7 @@ do_kernel_trap(CPUARMState *env)
     uint32_t addr;
     uint32_t cpsr;
     uint32_t val;
+    target_siginfo_t info;
 
     switch (env->regs[15]) {
     case 0xffff0fa0: /* __kernel_memory_barrier */
@@ -516,13 +517,16 @@ do_kernel_trap(CPUARMState *env)
         start_exclusive();
         cpsr = cpsr_read(env);
         addr = env->regs[2];
-        /* FIXME: This should SEGV if the access fails.  */
-        if (get_user_u32(val, addr))
-            val = ~env->regs[0];
+        if (get_user_u32(val, addr)) {
+            env->exception.vaddress = addr;
+            goto segv;
+        }
         if (val == env->regs[0]) {
             val = env->regs[1];
-            /* FIXME: Check for segfaults.  */
-            put_user_u32(val, addr);
+            if (put_user_u32(val, addr)) {
+                env->exception.vaddress = addr;
+                goto segv;
+            }
             env->regs[0] = 0;
             cpsr |= CPSR_C;
         } else {
@@ -551,6 +555,16 @@ do_kernel_trap(CPUARMState *env)
     env->regs[15] = addr;
 
     return 0;
+
+segv:
+    end_exclusive();
+    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);
+    return 0;
 }
 
 void cpu_loop(CPUARMState *env)
-- 
2.10.2

Re: [Qemu-devel] [RESEND PATCH] target-arm/abi32: check for segfault in do_kernel_trap

[Peter Maydell]

On 2 January 2017 at 12:44, Seraphime Kirkovski <kirkseraph@gmail.com> wrote:
> Currently, the cmpxchg implementation tests whether the destination address
> is readable:
>   - if it is, we read the value and continue with the comparison
>   - if isn't, i.e. access to addr would segfault, we assume that src != dest
>     rather than queuing a SIGSEGV.
>
> The same problem exists in the case where src == dest: the code doesn't
> check whether put_user_u32 succeeds.
>
> This fixes both problems by sending a SIGSEGV when the destination address
> is inaccessible.
>
> Signed-off-by: Seraphime Kirkovski <kirkseraph@gmail.com>
> ---
>  linux-user/main.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 75b199f..376b0c3 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -503,6 +503,7 @@ do_kernel_trap(CPUARMState *env)
>      uint32_t addr;
>      uint32_t cpsr;
>      uint32_t val;
> +    target_siginfo_t info;
>
>      switch (env->regs[15]) {
>      case 0xffff0fa0: /* __kernel_memory_barrier */
> @@ -516,13 +517,16 @@ do_kernel_trap(CPUARMState *env)
>          start_exclusive();
>          cpsr = cpsr_read(env);
>          addr = env->regs[2];
> -        /* FIXME: This should SEGV if the access fails.  */
> -        if (get_user_u32(val, addr))
> -            val = ~env->regs[0];
> +        if (get_user_u32(val, addr)) {
> +            env->exception.vaddress = addr;
> +            goto segv;
> +        }
>          if (val == env->regs[0]) {
>              val = env->regs[1];
> -            /* FIXME: Check for segfaults.  */
> -            put_user_u32(val, addr);
> +            if (put_user_u32(val, addr)) {
> +                env->exception.vaddress = addr;
> +                goto segv;
> +            }
>              env->regs[0] = 0;
>              cpsr |= CPSR_C;
>          } else {
> @@ -551,6 +555,16 @@ do_kernel_trap(CPUARMState *env)
>      env->regs[15] = addr;
>
>      return 0;
> +
> +segv:
> +    end_exclusive();
> +    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);
> +    return 0;

If you compare what happens with this segv code with
what happens for the segvs detected inside
arm_kernel_cmpxchg64_helper(), there's a difference.
With this code we will queue the signal and then return,
skipping the code which updates env->regs[15] and env->thumb.
The existing codepath for cmpxchg64 doesn't skip that code.
The effect is that for cmpxchg64 the SEGV signal handler
will see a PC pointing at the call into the kernel commpage,
whereas for this code it will see the PC actually in the
kernel commpage.

I'm not sure which of these options is the best choice,
but I do think we should be consistent.

>  }
>
>  void cpu_loop(CPUARMState *env)
> --
> 2.10.2

thanks
-- PMM

Re: [Qemu-devel] [RESEND PATCH] target-arm/abi32: check for segfault in do_kernel_trap

[Seraphime Kirkovski]

<riku.voipio@iki.fi>,qemu-arm <qemu-arm@nongnu.org>
Bcc:
Subject: Re: [Qemu-devel] [RESEND PATCH] target-arm/abi32: check for 
segfault in do_kernel_trap
Reply-To: In-Reply-To: 
<CAFEAcA-9aYRHcn1dAD0xuR9H8sJ__HOK9ayWmsK7H-Cr2rj4SQ@mail.gmail.com>

Hi Peter,
thanks for the feedback.

On Thu, Jan 05, 2017 at 02:42:38PM +0000, Peter Maydell wrote:
> If you compare what happens with this segv code with
> what happens for the segvs detected inside
> arm_kernel_cmpxchg64_helper(), there's a difference.
> With this code we will queue the signal and then return,
> skipping the code which updates env->regs[15] and env->thumb.
> The existing codepath for cmpxchg64 doesn't skip that code.
> The effect is that for cmpxchg64 the SEGV signal handler
> will see a PC pointing at the call into the kernel commpage,
> whereas for this code it will see the PC actually in the
> kernel commpage.
> 
> I'm not sure which of these options is the best choice,
> but I do think we should be consistent.

I think it would be safer to keep the current behaviour.

In terms of implementation, I find it would be better to extract the 
__kernel_cmpxchg code in a separate function. On the one hand, this 
would avoid code duplication or strange gotos and make do_kernel_trap
more readable. On the other hand, this would clutter a bit git history.
If it is acceptable, I will send a patch with those changes. If not, 
will try to find a cleaner solution without creating new functions.

Cheers,
Seraphime Kirkovski

Re: [Qemu-devel] [RESEND PATCH] target-arm/abi32: check for segfault in do_kernel_trap

[Peter Maydell]

On 6 January 2017 at 14:35, Seraphime Kirkovski <kirkseraph@gmail.com> wrote:
> In terms of implementation, I find it would be better to extract the
> __kernel_cmpxchg code in a separate function. On the one hand, this
> would avoid code duplication or strange gotos and make do_kernel_trap
> more readable. On the other hand, this would clutter a bit git history.
> If it is acceptable, I will send a patch with those changes. If not,
> will try to find a cleaner solution without creating new functions.

Refactoring the code sounds like a good idea to me -- go ahead.

thanks
-- PMM