linux-kernel - Re: [RFC][PATCH v2 4/5] x86/uaccess: Implement unsafe_try_cmpxchg

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]

Date:   Thu, 27 Jan 2022 23:33:12 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     mingo@...hat.com, tglx@...utronix.de, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        bristot@...hat.com, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, linux-api@...r.kernel.org, x86@...nel.org,
        pjt@...gle.com, posk@...gle.com, avagin@...gle.com,
        jannh@...gle.com, tdelisle@...terloo.ca, mark.rutland@....com,
        posk@...k.io, Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [RFC][PATCH v2 4/5] x86/uaccess: Implement
 unsafe_try_cmpxchg_user()

+Nick

On Thu, Jan 27, 2022, Peter Zijlstra wrote:
> On Thu, Jan 27, 2022 at 06:36:19AM +0000, Sean Christopherson wrote:
> > On Thu, Jan 27, 2022, Sean Christopherson wrote:
> > > Doh, I should have specified that KVM needs 8-byte CMPXCHG on 32-bit kernels due
> > > to using it to atomically update guest PAE PTEs and LTR descriptors (yay).
> > > 
> > > Also, KVM's use case isn't a tight loop, how gross would it be to add a slightly
> > > less unsafe version that does __uaccess_begin_nospec()?  KVM pre-checks the address
> > > way ahead of time, so the access_ok() check can be omitted.  Alternatively, KVM
> > > could add its own macro, but that seems a little silly.  E.g. somethign like this,
> > > though I don't think this is correct
> > 
> > *sigh*
> > 
> > Finally realized I forgot to add back the page offset after converting from guest
> > page frame to host virtual address.  Anyways, this is what I ended up with, will
> > test more tomorrow.
> 
> Looks about right :-) (famous last words etc..)

And it was right, but clang-13 ruined the party :-/

clang barfs on asm goto with a "+m" input/output.  Change the "+m" to "=m" and
clang is happy.  Remove usage of the label, clang is happy.

I tried a bunch of different variants to see if anything would squeak by, but
clang found a way to die on everything I threw at it.

$ clang --version

  Debian clang version 13.0.0-9+build1
  Target: x86_64-pc-linux-gnu
  Thread model: posix
  InstalledDir: /usr/bin

As written, with a named label param, clang yields:

  $ echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null
  <stdin>:1:29: error: invalid operand in inline asm: '.long (${1:l}) - .'
  int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }
                            ^
  <stdin>:1:29: error: unknown token in expression
  <inline asm>:1:9: note: instantiated into assembly here
          .long () - .
               ^
  2 errors generated.

While clang is perfectly happy switching "+m" to "=m":

  $ echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "=m"(*x) ::: bar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null

Referencing the label with a numbered param yields either the original error:

  $ echo 'int foo(int *x) { asm goto (".long (%l1) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null
  <stdin>:1:29: error: invalid operand in inline asm: '.long (${1:l}) - .'
  int foo(int *x) { asm goto (".long (%l1) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }
                            ^
  <stdin>:1:29: error: unknown token in expression
  <inline asm>:1:9: note: instantiated into assembly here
          .long () - .
                 ^
   2 errors generated.

Bumping the param number (more below) yields a different error (I tried defining
tmp1, that didn't work :-) ).

  $ echo 'int foo(int *x) { asm goto (".long (%l2) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null
  error: Undefined temporary symbol .Ltmp1
  1 error generated.

Regarding the param number, gcc also appears to have a goof with asm goto and "+m",
but bumping the param number in that case remedies its woes.

  $echo 'int foo(int *x) { asm goto (".long (%l1) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | gcc -x c - -c -o /dev/null
  <stdin>: In function ‘foo’:
  <stdin>:1:19: error: invalid 'asm': '%l' operand isn't a label

  $ echo 'int foo(int *x) { asm goto (".long (%l2) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | gcc -x c - -c -o /dev/null


So my immediate question: how do we want to we deal with this in the kernel?  Keeping
in mind that I'd really like to send this to stable@ to fix the KVM mess.

I can think of few options that are varying degrees of gross.

  1) Use a more complex sequence for probing CC_HAS_ASM_GOTO_OUTPUT.

  2) Use an output-only "=m" operand.

  3) Use an input register param.

Option #1 has the obvious downside of the fancier asm goto for  __get_user_asm()
and friends being collateral damage.  The biggest benefit is it'd reduce the
likelihood of someone else having to debug similar errors, which was quite painful.

Options #2 and #3 are quite gross, but I _think_ would be ok since the sequence
is tagged as clobbering memory anyways?