Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE

[Linus Torvalds]

On Mon, Jul 2, 2018 at 1:13 PM Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>
> Like this:
>
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 3b2490b81918..ec40223c8856 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -170,6 +170,26 @@ static void exit_to_usermode_loop(struct pt_regs
> *regs, u32 cached_flags)
>          if (cached_flags & _TIF_USER_RETURN_NOTIFY)
>              fire_user_return_notifiers();
>
> +        if (unlikely(!user_64bit_mode(regs) &&
> +                 (regs->ip & 0xffffffff00000000ull))) {

I'd be afraid that code generation is atrocious. So more something like this:

    static noinline send_sigsegv(..) { }

    ...
    if (unlikely(!user_64bit_mode(regs)) {
        if (unlikely(*(1+(u32 *)&regs->ip)))
            send_sigsegv(tsk);

to make sure it doesn't do crazy big constants in the normal path, and
doesn't allocate silly stack frames.

But as mentioned, I'm not entirely convinced this is worth it. But I
wasn't sure it's worth it for rseq.

So basically, *if* we do these kinds of checks, I'd personally rather
do a *generic* "we don't return to garbage 64-bit values in compat
mode" than have special case code that is only for rseq and is truly
irrelevant to all normal cases.

The generic case might even be worth a test-case. And if we do that,
we should probably check that we don't do something odd like
sign-extending the %rip value we load from stack for signal return
etc, that just happens to work because nobody cared about the upper
bits.

So there's a lot of these kinds of small details that are of
questionable importance, but might in theory be worth it.

                  Linus