• Sebastian Andrzej Siewior's avatar
    x86/fpu: Don't cache access to fpu_fpregs_owner_ctx · 59c4bd85
    Sebastian Andrzej Siewior authored
    The state/owner of the FPU is saved to fpu_fpregs_owner_ctx by pointing
    to the context that is currently loaded. It never changed during the
    lifetime of a task - it remained stable/constant.
    
    After deferred FPU registers loading until return to userland was
    implemented, the content of fpu_fpregs_owner_ctx may change during
    preemption and must not be cached.
    
    This went unnoticed for some time and was now noticed, in particular
    since gcc 9 is caching that load in copy_fpstate_to_sigframe() and
    reusing it in the retry loop:
    
      copy_fpstate_to_sigframe()
        load fpu_fpregs_owner_ctx and save on stack
        fpregs_lock()
        copy_fpregs_to_sigframe() /* failed */
        fpregs_unlock()
             *** PREEMPTION, another uses FPU, changes fpu_fpregs_owner_ctx ***
    
        fault_in_pages_writeable() /* succeed, retry */
    
        fpregs_lock()
    	__fpregs_load_activate()
    	  fpregs_state_valid() /* uses fpu_fpregs_owner_ctx from stack */
        copy_fpregs_to_sigframe() /* succeeds, random FPU content */
    
    This is a comparison of the assembly produced by gcc 9, without vs with this
    patch:
    
    | # arch/x86/kernel/fpu/signal.c:173:      if (!access_ok(buf, size))
    |        cmpq    %rdx, %rax      # tmp183, _4
    |        jb      .L190   #,
    |-# arch/x86/include/asm/fpu/internal.h:512:       return fpu == this_cpu_read_stable(fpu_fpregs_owner_ctx) && cpu == fpu->last_cpu;
    |-#APP
    |-# 512 "arch/x86/include/asm/fpu/internal.h" 1
    |-       movq %gs:fpu_fpregs_owner_ctx,%rax      #, pfo_ret__
    |-# 0 "" 2
    |-#NO_APP
    |-       movq    %rax, -88(%rbp) # pfo_ret__, %sfp
    …
    |-# arch/x86/include/asm/fpu/internal.h:512:       return fpu == this_cpu_read_stable(fpu_fpregs_owner_ctx) && cpu == fpu->last_cpu;
    |-       movq    -88(%rbp), %rcx # %sfp, pfo_ret__
    |-       cmpq    %rcx, -64(%rbp) # pfo_ret__, %sfp
    |+# arch/x86/include/asm/fpu/internal.h:512:       return fpu == this_cpu_read(fpu_fpregs_owner_ctx) && cpu == fpu->last_cpu;
    |+#APP
    |+# 512 "arch/x86/include/asm/fpu/internal.h" 1
    |+       movq %gs:fpu_fpregs_owner_ctx(%rip),%rax        # fpu_fpregs_owner_ctx, pfo_ret__
    |+# 0 "" 2
    |+# arch/x86/include/asm/fpu/internal.h:512:       return fpu == this_cpu_read(fpu_fpregs_owner_ctx) && cpu == fpu->last_cpu;
    |+#NO_APP
    |+       cmpq    %rax, -64(%rbp) # pfo_ret__, %sfp
    
    Use this_cpu_read() instead this_cpu_read_stable() to avoid caching of
    fpu_fpregs_owner_ctx during preemption points.
    
    The Fixes: tag points to the commit where deferred FPU loading was
    added. Since this commit, the compiler is no longer allowed to move the
    load of fpu_fpregs_owner_ctx somewhere else / outside of the locked
    section. A task preemption will change its value and stale content will
    be observed.
    
     [ bp: Massage. ]
    Debugged-by: default avatarAustin Clements <austin@google.com>
    Debugged-by: default avatarDavid Chase <drchase@golang.org>
    Debugged-by: default avatarIan Lance Taylor <ian@airs.com>
    Fixes: 5f409e20 ("x86/fpu: Defer FPU state load until return to userspace")
    Signed-off-by: default avatarSebastian Andrzej Siewior <bigeasy@linutronix.de>
    Signed-off-by: default avatarBorislav Petkov <bp@suse.de>
    Reviewed-by: default avatarRik van Riel <riel@surriel.com>
    Tested-by: default avatarBorislav Petkov <bp@suse.de>
    Cc: Aubrey Li <aubrey.li@intel.com>
    Cc: Austin Clements <austin@google.com>
    Cc: Barret Rhoden <brho@google.com>
    Cc: Dave Hansen <dave.hansen@intel.com>
    Cc: David Chase <drchase@golang.org>
    Cc: "H. Peter Anvin" <hpa@zytor.com>
    Cc: ian@airs.com
    Cc: Ingo Molnar <mingo@redhat.com>
    Cc: Josh Bleecher Snyder <josharian@gmail.com>
    Cc: Thomas Gleixner <tglx@linutronix.de>
    Cc: x86-ml <x86@kernel.org>
    Link: https://lkml.kernel.org/r/20191128085306.hxfa2o3knqtu4wfn@linutronix.de
    Link: https://bugzilla.kernel.org/show_bug.cgi?id=205663
    59c4bd85
internal.h 16.6 KB