Re: [PATCH v4 05/25] linux-user/arm: Implement setup_sigtramp

[Peter Maydell]

On Tue, 28 Sept 2021 at 03:00, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Mirror what the kernel does in arch/arm/kernel/signal.h,
> using the old sigframe struct in the rt sigframe struct.
>
> Update the trampoline code to match the kernel: this uses
> sp-relative accesses rather than pc-relative.
>
> Copy the code into frame->retcode from the trampoline page.
> This minimises the different cases wrt arm vs thumb vs fdpic.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


> @@ -225,44 +204,34 @@ setup_return(CPUARMState *env, struct target_sigaction 
> *ka,
>
>      if (ka->sa_flags & TARGET_SA_RESTORER) {
>          if (is_fdpic) {
> -            /* For FDPIC we ensure that the restorer is called with a
> -             * correct r9 value.  For that we need to write code on
> -             * the stack that sets r9 and jumps back to restorer
> -             * value.
> -             */
> -            if (thumb) {
> -                __put_user(sigreturn_fdpic_thumb_codes[0], rc);
> -                __put_user(sigreturn_fdpic_thumb_codes[1], rc + 1);
> -                __put_user(sigreturn_fdpic_thumb_codes[2], rc + 2);
> -                __put_user((abi_ulong)ka->sa_restorer, rc + 3);
> -            } else {
> -                __put_user(sigreturn_fdpic_codes[0], rc);
> -                __put_user(sigreturn_fdpic_codes[1], rc + 1);
> -                __put_user(sigreturn_fdpic_codes[2], rc + 2);
> -                __put_user((abi_ulong)ka->sa_restorer, rc + 3);
> -            }
> -
> -            retcode = rc_addr + thumb;
> +            __put_user((abi_ulong)ka->sa_restorer, &frame->retcode[3]);
> +            retcode = (sigreturn_fdpic_tramp +
> +                       retcode_idx * RETCODE_BYTES + thumb);

Here 'retcode' is an interworking-PC value with the LSB indicating
Thumb mode...

> +            copy_retcode = true;
>          } else {
>              retcode = ka->sa_restorer;
> +            copy_retcode = false;
>          }
>      } else {
> -        unsigned int idx = thumb;
> +        retcode = default_sigreturn + retcode_idx * RETCODE_BYTES + thumb;
> +        copy_retcode = true;
> +    }
>
> -        if (ka->sa_flags & TARGET_SA_SIGINFO) {
> -            idx += 2;
> +    /* Copy the code to the stack slot for ABI compatibility. */
> +    if (copy_retcode) {
> +        uint32_t *host_rc = g2h_untagged(retcode);

...but here we treat it as a normal guest address that we can
convert into a host address and dereference. If the signal handler
is being entered in Thumb mode this will be a misaligned pointer.

> +        int i;
> +
> +        for (i = 0; i < RETCODE_WORDS; ++i) {
> +            __put_user(host_rc[i], &frame->retcode[i]);
>          }
> -
> -        __put_user(retcodes[idx], rc);
> -
> -        retcode = rc_addr + thumb;
>      }
>
>      env->regs[0] = usig;
>      if (is_fdpic) {
>          env->regs[9] = handler_fdpic_GOT;
>      }
> -    env->regs[13] = frame_addr;
> +    env->regs[13] = sp_addr;
>      env->regs[14] = retcode;
>      env->regs[15] = handler & (thumb ? ~1 : ~3);
>      cpsr_write(env, cpsr, CPSR_IT | CPSR_T | CPSR_E, CPSRWriteByInstr);

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM