Re: [Qemu-arm] [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 <address@hidden> 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 <address@hidden>
> ---
>  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