Message-ID: <CAHk-=whUJTQuLorG=pRv1XAe_=9oWrVFkYb26q87fhkqDs5zcQ@mail.gmail.com>
Date:   Wed, 22 Jun 2022 11:57:44 -0500
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Zijlstra <peterz@...radead.org>,
        Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: some apparently valid objtool clang warnings

So due to another thread, I'm doing a full allmodconfig clang build
while on the road, and I'm getting a few objtool warnings that I don't
get with gcc.

Now, some of them are probably just due to the usual "clang generates
different code and has that nasty fall-through behavior when for
non-returning functions".

But I note that some of them actually seem to be valid and signs of real issues.

In particular, the

  call to __ubsan_handle_load_invalid_value() with UACCESS enabled

warnings tend to be a real sign that somebody is doing something very
wrong inside a user access region, and kvm seems to be buggy here.

I get it for

  emulator_cmpxchg_emulated+0x6c2
  paging64_update_accessed_dirty_bits+0x361
  ept_update_accessed_dirty_bits+0x3d0

and at least the emulator_cmpxchg_emulated() case seems to be due to
an actual bug (or at least misfeature) of __try_cmpxchg_user() and the
way kvm uses it.

In particular, kvm does

  #define emulator_try_cmpxchg_user(t, ptr, old, new) \
        (__try_cmpxchg_user((t __user *)(ptr), (t *)(old), *(t
*)(new), efault ## t))

and look at that third argument: "*(t *)(new)". It is doing a pointer
dereference.

And then when you look at the __try_cmpxchg_user(), it will pass that
argument down without evaluating it, and do so inside the
__uaccess_begin_nospec()/__uaccess_end() region.

It will pass it down to the unsafe_try_cmpxchg_user() macro, which
will pass it down to the appropriate __try_cmpxchg_user_asm() macro,
and only inside *that* macro will it then do

    __typeof__(*(_ptr)) __old = *_old;
    __typeof__(*(_ptr)) __new = (_new);

and *both* of those lines are buggy, since they both do memory
accesses that are not to user space (the first because of the '*_old'
dereference, and the second because of the deference in the macro
argument), and should have been done outside the
__uaccess_begin_nospec region.

I'm not sure why gcc doesn't see this warning, but it might be random
code generation, or maybe objtool has explicit code to hide this for
gcc. But it does look buggy, and the clang warning appears real.

We also do have that

        if (unlikely(!success))
                *_old = __old;

inside __try_cmpxchg_user_asm after the actual asm that also seems
buggy and wrong for the exact same reason - it shouldn't be done
within the STAC region.

Now, all of these macros results in code that *works*, and it's not
fatal in that sense. But it does seem to be very wrong anyway.

The update_accessed_dirty_bits() cases seem to be the exact same
thing: it's __try_cmpxchg_user() in just another place.

Comments? I think those old/new things should be moved out one macro
level, and be done inside __try_cmpxchg_user() itself, outside the
uaccess region.

That may require some games for the end-game where we do that "assign
the _old value", and maybe the __uaccess_end needs to be moved into
the success case. But it would be good to do this right. No?

            Linus

Powered by blists - more mailing lists