Re: [PATCH] powerpc/signal64: Don't read sigaction arguments back from user memory

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Michael Ellerman's message of June 10, 2021 5:29 pm:
> When delivering a signal to a sigaction style handler (SA_SIGINFO), we
> pass pointers to the siginfo and ucontext via r4 and r5.
> 
> Currently we populate the values in those registers by reading the
> pointers out of the sigframe in user memory, even though the values in
> user memory were written by the kernel just prior:
> 
>   unsafe_put_user(&frame->info, &frame->pinfo, badframe_block);
>   unsafe_put_user(&frame->uc, &frame->puc, badframe_block);
>   ...
>   if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
>   	err |= get_user(regs->gpr[4], (unsigned long __user *)&frame->pinfo);
>   	err |= get_user(regs->gpr[5], (unsigned long __user *)&frame->puc);
> 
> ie. we write &frame->info into frame->pinfo, and then read frame->pinfo
> back into r4, and similarly for &frame->uc.
> 
> The code has always been like this, since linux-fullhistory commit
> d4f2d95eca2c ("Forward port of 2.4 ppc64 signal changes.").
> 
> There's no reason for us to read the values back from user memory,
> rather than just setting the value in the gpr[4/5] directly. In fact
> reading the value back from user memory opens up the possibility of
> another user thread changing the values before we read them back.
> Although any process doing that would be racing against the kernel
> delivering the signal, and would risk corrupting the stack, so that
> would be a userspace bug.
> 
> Note that this is 64-bit only code, so there's no subtlety with the size
> of pointers differing between kernel and user. Also the frame variable
> is not modified to point elsewhere during the function.
> 
> In the past reading the values back from user memory was not costly, but
> now that we have KUAP on some CPUs it is, so we'd rather avoid it for
> that reason too.
> 
> So change the code to just set the values directly, using the same
> values we have written to the sigframe previously in the function.
> 
> Note also that this matches what our 32-bit signal code does.
> 
> Using a version of will-it-scale's signal1_threads that sets SA_SIGINFO,
> this results in a ~4% increase in signals per second on a Power9, from
> 229,777 to 239,766.

Good find, nice improvement. Will make it possible to make the error
handling much nicer too I think.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

You've moved copy_siginfo_to_user right up to the user access unlock,
could save 2 more KUAP lock/unlocks if we had an unsafe_clear_user. If
we can move the other user access stuff up as well, the stack frame
put_user could use unsafe_put_user as well, saving 1 more. Another few
percent?


> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/signal_64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index dca66481d0c2..f58e7a98d0df 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -948,8 +948,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  	regs->gpr[3] = ksig->sig;
>  	regs->result = 0;
>  	if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
> -		err |= get_user(regs->gpr[4], (unsigned long __user *)&frame->pinfo);
> -		err |= get_user(regs->gpr[5], (unsigned long __user *)&frame->puc);
> +		regs->gpr[4] = (unsigned long)&frame->info;
> +		regs->gpr[5] = (unsigned long)&frame->uc;
>  		regs->gpr[6] = (unsigned long) frame;
>  	} else {
>  		regs->gpr[4] = (unsigned long)&frame->uc.uc_mcontext;
> -- 
> 2.25.1
> 
>