Message-ID: <CAHk-=wizu7DA7EDrsHQLmkTFBvCRxNyPMHaeMDYMF_U75s9RvQ@mail.gmail.com>
Date:   Wed, 1 Jul 2020 12:35:33 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: objtool clac/stac handling change..

On Wed, Jul 1, 2020 at 11:29 AM Andy Lutomirski <luto@...nel.org> wrote:
>
> Do we really want the exception handling to do the CLAC?  Having
> unsafe_get_user() do CLAC seems surprising to me, and it will break
> use cases like:
>
> if (!user_access_begin(...)
>   goto out;
>
> ret = unsafe_get_user(...);
>
> user_access_end();
>
> check ret;

That's not how unsafe_get_user() works.

unsafe_get_user() always jumps to the error label, it never returns a
value. So the code is actually now what you claim above, but

    if (!user_access_begin(...)
       goto out;

    unsafe_get_user(..., out_fault);
    user_access_end();
   .. this is good, use the value we got..

out_fault:
    user_access_end();
out:
    return -EFAULT;

And that's _exactly_ why I'd like to make the change: because that
means that the error label for a user access failure is exactly the
same as the error label for the "user_access_begin()" failure.

So with my suggestion, that "out_fault" label and extra
user_access_end() would go away, and only "out" would remain.

So my suggestion actually simplifies the use cases, and your example
was literally an argument _for_ the change, not against it.

That's not why I started doing it, though. The real simplification is
inside the low-level implementation.

Right now __get_user_nocheck() does this:

        __uaccess_begin();                                      \
        __get_user_size(__gu_val, __gu_ptr, __gu_size, __gu_err);       \
        __uaccess_end();                                                \
        (x) = (__force __typeof__(*(ptr)))__gu_val;                     \
        __builtin_expect(__gu_err, 0);                                  \

because __get_user_nocheck() internally does *not* use the jumping
version (yet) because of the fact how gcc can't do "asm goto" with
outputs.

And it's actually _important_ that the assignment to "x" is done
outside the user access region (because "x" can be a complex
expression).

But look at what happens if we change things to use a exception jump
instead of that __gu_error value.

Then we want the code to become this:

        __uaccess_begin_nospec();                                       \
        __get_user_size(__gu_val, __gu_ptr, __gu_size, __gu_label);     \
        __uaccess_end();                                                \
        (x) = (__force __typeof__(*(ptr)))__gu_val;                     \
        __gu_ret = 0;                                                   \
__gu_label:                                                             \
        __builtin_expect(__gu_err, 0);                                  \

and that actually looks nice and understandable, and the compiler will
also have a really easy time turing any subsequent test of the return
value into a trivial "we know the fallthrough case returned zero"
because instead of setting "__gu_err" much deeper and dynamically (and
with other code in between), it gets set right before the return from
that statement expression macro.

Btw, all the "it makes it easier to implement" is doubly true in all
the low-level asm code too. Go look into arch/x86/lib/{get,put}user.S
right now, and see how the error cases all have two different
entrypoints, and we have code like

SYM_CODE_START_LOCAL(.Lbad_get_user_clac)
        ASM_CLAC
bad_get_user:
        xor %edx,%edx
        mov $(-EFAULT),%_ASM_AX
        ret
SYM_CODE_END(.Lbad_get_user_clac)

where that "Lbad_get_user_clac" label is used for the exception case,
and the "bad_get_user" label is used for the "bad address" case.

So it
 (a) makes it easier for users
 (b) makes it easier to implement
 (c) will actually make it easier for compilers to optimize too

Now, if the exception does *not* do the "stop user accesses", we have
to continue doing the two different failure targets (both in C code
and in asm code), and for the __get_user_nocheck() case where we use
"asm goto" with outputs, we can do things like this:

        __uaccess_begin_nospec();                                       \
        __get_user_size(__gu_val, __gu_ptr, __gu_size, __gu_label);     \
        __uaccess_end();                                                \
        (x) = (__force __typeof__(*(ptr)))__gu_val;                     \
        __gu_err = 0;                                                   \
        if (0) {                                                        \
 __gu_label:                                                            \
                __uaccess_end();                                        \
                __gu_err = -EFAULT;                                     \
        }                                                               \
        __builtin_expect(__gu_err, 0);                                  \

which certainly works, but I think you'll admit it's ugly (note that
nice "if (0)" trick to get the separate error case that does that
__uaccess_end() that can't be done in the common end part because that
complex access has to be done after closing the user access region).

So this is why I'd like to change the rules. A user access fault would
close the user access window.

              Linus

Powered by blists - more mailing lists