Messages in this thread | | | From | Linus Torvalds <> | Date | Wed, 22 Jun 2022 11:57:44 -0500 | 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
| |