linux-kernel - Re: some apparently valid objtool clang warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 23 Jun 2022 14:42:38 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Sean Christopherson <seanjc@...gle.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     the arch/x86 maintainers <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: some apparently valid objtool clang warnings

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