Re: some apparently valid objtool clang warnings

[Paolo Bonzini]

On Wed, Jun 22, 2022 at 6:58 PM Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 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