Messages in this thread | | | Date | Thu, 23 Jun 2022 14:42:38 +0200 | From | Paolo Bonzini <> | Subject | Re: 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
| |