lkml.org 
[lkml]   [2022]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: some apparently valid objtool clang warnings
On Wed, Jun 22, 2022 at 6:58 PM Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 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.
> 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 [...] inside the __uaccess_begin_nospec()/__uaccess_end()
> region [...] and *both* of those lines are buggy, since they both do
> memory accesses that are not to user space [...] 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.
>
> 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?

Yes, I agree that __try_cmpxchg_user should look more like this:

/* "Returns" 0 on success, 1 on failure, -EFAULT if the access faults. */
#define __try_cmpxchg_user(_ptr, _oldp, _nval, _label) ({ \
int __ret = -EFAULT; \
+ __typeof__(_ptr) __ptr = (_ptr); \
+ __typeof__(_ptr) __pold = (__typeof__(_ptr))(_oldp); \
+ __typeof__(*(_ptr)) __old = *__pold; \
+ __typeof__(*(_ptr)) __new = (_nval); \
__uaccess_begin_nospec(); \
- __ret = !unsafe_try_cmpxchg_user(_ptr, _oldp, _nval, _label); \
+ __ret = !____try_cmpxchg_user(__ptr, __old, __new, _label); \
_label: \
__uaccess_end(); \
+ if (unlikely(!__result)) \
+ *__pold = __old; \
__ret; \
})
(where I have renamed unsafe_try_cmpxchg_user because it doesn't
anymore write to *_pold; it's not a full try_cmpxchg). I'll
clean up the rest of the macros and send it out as a patch.

Paolo

\
 
 \ /
  Last update: 2022-06-23 14:43    [W:1.660 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site