linux-kernel - Re: [PATCH] x86: fix user address masking non-canonical speculation issue

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHk-=wjBkvHNTy3orkjw=2GH25S4MSFWesSjni2zZNW2+gjomg@mail.gmail.com>
Date: Thu, 24 Oct 2024 11:13:35 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Borislav Petkov <bp@...en8.de>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, x86@...nel.org, 
	Andrew Cooper <andrew.cooper3@...rix.com>, Josh Poimboeuf <jpoimboe@...nel.org>
Subject: Re: [PATCH] x86: fix user address masking non-canonical speculation issue

On Thu, 24 Oct 2024 at 10:53, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> >
> >         asm("cmp %[ptr],%[ret]\n\t"
> >             "sbb %[ret],%[ret]\n\t"
> >             "or  %[ptr],%[ret]"
> >                 : [ret] "=r" (ret)
> >                 : [ptr] "r" (ptr),
> >                   "0" (runtime_const_ptr(USER_PTR_MAX)));
>
> Will do at least for the newlines.

Actually, I should also just remove the "or" and move that into C
code. It will allow the compiler to decide which way it wants to do
the bitwise 'or', which means that the compiler can pick whichever
output register is a better choice.

Probably never matters in practice, but leaving decisions like that to
the compiler and avoiding one more fixed asm instruction is a good
thing.

It does result in a few more casts on the C side, since you can't just
do bitwise 'or' on a pointer, but I think it's still the right thing
to do. So that thing becomes

  static inline void __user *mask_user_address(const void __user *ptr)
  {
        unsigned long mask;
        asm("cmp %1,%0\n\t"
            "sbb %0,%0"
                :"=r" (mask)
                :"r" (ptr),
                 "0" (runtime_const_ptr(USER_PTR_MAX)));
        return (__force void __user *)(mask | (__force unsigned long)ptr);
  }

which I'm certainly not claiming is a thing of beauty, but the
generated code looks ok if you just ignore the #APP/#NOAPP noise:

  # ./arch/x86/include/asm/uaccess_64.h:71:                "0"
(runtime_const_ptr(USER_PTR_MAX)));
  #APP
  # 71 "./arch/x86/include/asm/uaccess_64.h" 1
        mov $81985529216486895,%rax     #, __ret
  1:
  .pushsection runtime_ptr_USER_PTR_MAX,"a"
        .long 1b - 8 - .        #
        .popsection
  # 0 "" 2
  # lib/strncpy_from_user.c:114: {
  #NO_APP
        pushq   %rbx    #
        movq    %rdi, %r9       # tmp149, dst
        movq    %rdx, %r11      # tmp151, count
  # ./arch/x86/include/asm/uaccess_64.h:67:       asm("cmp %1,%0\n\t"
  #APP
  # 67 "./arch/x86/include/asm/uaccess_64.h" 1
        cmp %rsi,%rax   # src, mask
        sbb %rax,%rax   # mask
  # 0 "" 2
  # ./arch/x86/include/asm/uaccess_64.h:72:       return (__force void
__user *)(mask | (__force unsigned long)ptr);
  #NO_APP
        orq     %rax, %rsi      # mask, _44

so you actually see gcc filling in variable names etc (well "variable
names" may be a bit generous: "_44" is a pseudo for the new value of
src, but that's just how compilers are with SSA - assignments create a
whole new temporary).

So legibility is very much in the eye of the beholder. You have to be
pretty damn used to looking at the generated asm to find any of this
even remotely legible.

                Linus