Re: [Qemu-devel] [PATCH v6 1/6] linux-user/aarch64: Reset btype for syscalls and signalsy

[Dave Martin]

On Wed, Jun 05, 2019 at 09:57:01PM +0100, Richard Henderson wrote:
> The value of btype for syscalls is CONSTRAINED UNPREDICTABLE,
> so we need to make sure that the value is 0 before clone,
> fork, or syscall return.
> 
> The kernel sets btype for the signal handler as if for a call.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  linux-user/aarch64/cpu_loop.c |  7 +++++++
>  linux-user/aarch64/signal.c   | 10 ++++++++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
> index 2f2f63e3e8..1f68b13168 100644
> --- a/linux-user/aarch64/cpu_loop.c
> +++ b/linux-user/aarch64/cpu_loop.c
> @@ -86,6 +86,13 @@ void cpu_loop(CPUARMState *env)
>  
>          switch (trapnr) {
>          case EXCP_SWI:
> +            /*
> +             * The state of BTYPE on syscall entry is CONSTRAINED
> +             * UNPREDICTABLE.  The real kernel will need to tidy this up
> +             * as well.  Do this before syscalls so that the value is
> +             * correct on return from syscall (especially clone & fork).
> +             */
> +            env->btype = 0;

Note, after discussion with the architects, I think Linux won't bother
sanitising this field in my next spin of the patches.

If the SVC (or HVC or SMC) sits in a PROT_BTI page, then it won't
execute anyway unless BTYPE is already 0: otherwise, a branch target
exception would be taken instead.

If the insn isn't in a PROT_BTI page, then BTYPE could be nonzero when
the SVC (etc.) exception is taken, but it also won't matter in that case
what BTYPE is on return, since branch target exceptions are never
generated on insns in non-guarded pages.

For this to make a difference:

 a) the SVC be in a non-PROT_BTI page, just before a page boundary,
    where the next page is a PROT_BTI page, so that the exception return
    goes to the next page.

 b) the SVC must be an mprotect() call or similar that enabled PROT_BTI
    for the patch containing the SVC itself.

These are both silly things to do, and we probably don't care what
happens in such cases.

(Question: does qemu ever mprotect() the page containing PC?  I'd hope
not...)

>              ret = do_syscall(env,
>                               env->xregs[8],
>                               env->xregs[0],
> diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
> index f84a9cf28a..5605d404b3 100644
> --- a/linux-user/aarch64/signal.c
> +++ b/linux-user/aarch64/signal.c
> @@ -506,10 +506,16 @@ static void target_setup_frame(int usig, struct 
> target_sigaction *ka,
>              + offsetof(struct target_rt_frame_record, tramp);
>      }
>      env->xregs[0] = usig;
> -    env->xregs[31] = frame_addr;
>      env->xregs[29] = frame_addr + fr_ofs;
> -    env->pc = ka->_sa_handler;
>      env->xregs[30] = return_addr;
> +    env->xregs[31] = frame_addr;
> +    env->pc = ka->_sa_handler;
> +
> +    /* Invoke the signal handler as if by indirect call.  */
> +    if (cpu_isar_feature(aa64_bti, arm_env_get_cpu(env))) {
> +        env->btype = 2;
> +    }
> +

Ack.  I had a simple test for this for native userspace, and it seems to
work as desired -- so I don't expect to change it.

Cheers
---Dave