Re: [PATCH 4/4] linux-user/arm: Fix identification of syscall numbers

[Edgar E. Iglesias]

On Mon, Apr 20, 2020 at 10:22:06PM +0100, Peter Maydell wrote:
> Our code to identify syscall numbers has some issues:
>  * for Thumb mode, we never need the immediate value from the insn,
>    but we always read it anyway
>  * bad immediate values in the svc insn should cause a SIGILL, but we
>    were abort()ing instead (via "goto error")
> 
> We can fix both these things by refactoring the code that identifies
> the syscall number to more closely follow the kernel COMPAT_OABI code:
>  * for Thumb it is always r7
>  * for Arm, if the immediate value is 0, then this is an EABI call
>    with the syscall number in r7
>  * otherwise, we XOR the immediate value with 0x900000
>    (ARM_SYSCALL_BASE for QEMU; __NR_OABI_SYSCALL_BASE in the kernel),
>    which converts valid syscall immediates into the desired value,
>    and puts all invalid immediates in the range 0x100000 or above
>  * then we can just let the existing "value too large, deliver
>    SIGILL" case handle invalid numbers, and drop the 'goto error'


I guess the -2 vs -4 issue has propagated into this patch but with that fixed:

Reviewed-by: Edgar E. Iglesias <address@hidden>



> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> You might prefer to read this patch with an "ignore whitespace
> changes" diff, as a big chunk of code is no longer inside an if()
> and got re-indented out one level.
> ---
>  linux-user/arm/cpu_loop.c | 143 ++++++++++++++++++++------------------
>  1 file changed, 77 insertions(+), 66 deletions(-)
> 
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index f042108b0be..eeb042829e2 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -299,85 +299,96 @@ void cpu_loop(CPUARMState *env)
>                  env->eabi = 1;
>                  /* system call */
>                  if (env->thumb) {
> -                    /* FIXME - what to do if get_user() fails? */
> -                    get_user_code_u16(insn, env->regs[15] - 2, env);
> -                    n = insn & 0xff;
> +                    /* Thumb is always EABI style with syscall number in r7 
> */
> +                    n = env->regs[7];
>                  } else {
> +                    /*
> +                     * Equivalent of kernel CONFIG_OABI_COMPAT: read the
> +                     * Arm SVC insn to extract the immediate, which is the
> +                     * syscall number in OABI.
> +                     */
>                      /* FIXME - what to do if get_user() fails? */
>                      get_user_code_u32(insn, env->regs[15] - 4, env);
>                      n = insn & 0xffffff;
> -                }
> -
> -                if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
> -                    /* linux syscall */
> -                    if (env->thumb || n == 0) {
> +                    if (n == 0) {
> +                        /* zero immediate: EABI, syscall number in r7 */
>                          n = env->regs[7];
>                      } else {
> -                        n -= ARM_SYSCALL_BASE;
> +                        /*
> +                         * This XOR matches the kernel code: an immediate
> +                         * in the valid range (0x900000 .. 0x9fffff) is
> +                         * converted into the correct EABI-style syscall
> +                         * number; invalid immediates end up as values
> +                         * > 0xfffff and are handled below as out-of-range.
> +                         */
> +                        n ^= ARM_SYSCALL_BASE;
>                          env->eabi = 0;
>                      }
> -                    if ( n > ARM_NR_BASE) {
> -                        switch (n) {
> -                        case ARM_NR_cacheflush:
> -                            /* nop */
> -                            break;
> -                        case ARM_NR_set_tls:
> -                            cpu_set_tls(env, env->regs[0]);
> -                            env->regs[0] = 0;
> -                            break;
> -                        case ARM_NR_breakpoint:
> -                            env->regs[15] -= env->thumb ? 2 : 4;
> -                            goto excp_debug;
> -                        case ARM_NR_get_tls:
> -                            env->regs[0] = cpu_get_tls(env);
> -                            break;
> -                        default:
> -                            if (n < 0xf0800) {
> -                                /*
> -                                 * Syscalls 0xf0000..0xf07ff (or 0x9f0000..
> -                                 * 0x9f07ff in OABI numbering) are defined
> -                                 * to return -ENOSYS rather than raising
> -                                 * SIGILL. Note that we have already
> -                                 * removed the 0x900000 prefix.
> -                                 */
> -                                qemu_log_mask(LOG_UNIMP,
> -                                    "qemu: Unsupported ARM syscall: 0x%x\n",
> -                                              n);
> -                                env->regs[0] = -TARGET_ENOSYS;
> +                }
> +
> +                if (n > ARM_NR_BASE) {
> +                    switch (n) {
> +                    case ARM_NR_cacheflush:
> +                        /* nop */
> +                        break;
> +                    case ARM_NR_set_tls:
> +                        cpu_set_tls(env, env->regs[0]);
> +                        env->regs[0] = 0;
> +                        break;
> +                    case ARM_NR_breakpoint:
> +                        env->regs[15] -= env->thumb ? 2 : 4;
> +                        goto excp_debug;
> +                    case ARM_NR_get_tls:
> +                        env->regs[0] = cpu_get_tls(env);
> +                        break;
> +                    default:
> +                        if (n < 0xf0800) {
> +                            /*
> +                             * Syscalls 0xf0000..0xf07ff (or 0x9f0000..
> +                             * 0x9f07ff in OABI numbering) are defined
> +                             * to return -ENOSYS rather than raising
> +                             * SIGILL. Note that we have already
> +                             * removed the 0x900000 prefix.
> +                             */
> +                            qemu_log_mask(LOG_UNIMP,
> +                                "qemu: Unsupported ARM syscall: 0x%x\n",
> +                                          n);
> +                            env->regs[0] = -TARGET_ENOSYS;
> +                        } else {
> +                            /*
> +                             * Otherwise SIGILL. This includes any SWI with
> +                             * immediate not originally 0x9fxxxx, because
> +                             * of the earlier XOR.
> +                             */
> +                            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 {
> -                                /* Otherwise SIGILL */
> -                                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 -= 2;
> -                                }
> -                                queue_signal(env, info.si_signo,
> -                                             QEMU_SI_FAULT, &info);
> +                                info._sifields._sigfault._addr -= 2;
>                              }
> -                            break;
> -                        }
> -                    } else {
> -                        ret = do_syscall(env,
> -                                         n,
> -                                         env->regs[0],
> -                                         env->regs[1],
> -                                         env->regs[2],
> -                                         env->regs[3],
> -                                         env->regs[4],
> -                                         env->regs[5],
> -                                         0, 0);
> -                        if (ret == -TARGET_ERESTARTSYS) {
> -                            env->regs[15] -= env->thumb ? 2 : 4;
> -                        } else if (ret != -TARGET_QEMU_ESIGRETURN) {
> -                            env->regs[0] = ret;
> +                            queue_signal(env, info.si_signo,
> +                                         QEMU_SI_FAULT, &info);
>                          }
> +                        break;
>                      }
>                  } else {
> -                    goto error;
> +                    ret = do_syscall(env,
> +                                     n,
> +                                     env->regs[0],
> +                                     env->regs[1],
> +                                     env->regs[2],
> +                                     env->regs[3],
> +                                     env->regs[4],
> +                                     env->regs[5],
> +                                     0, 0);
> +                    if (ret == -TARGET_ERESTARTSYS) {
> +                        env->regs[15] -= env->thumb ? 2 : 4;
> +                    } else if (ret != -TARGET_QEMU_ESIGRETURN) {
> +                        env->regs[0] = ret;
> +                    }
>                  }
>              }
>              break;
> -- 
> 2.20.1
> 
>