From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> To: Barret Rhoden <brho@google.com> Cc: Borislav Petkov <bp@alien8.de>, Josh Bleecher Snyder <josharian@gmail.com>, Rik van Riel <riel@surriel.com>, x86@kernel.org, linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, ian@airs.com, Austin Clements <austin@google.com>, David Chase <drchase@golang.org> Subject: [PATCH v2] x86/fpu: Don't cache access to fpu_fpregs_owner_ctx Date: Thu, 28 Nov 2019 09:53:06 +0100 Message-ID: <20191128085306.hxfa2o3knqtu4wfn@linutronix.de> (raw) In-Reply-To: <f4d5ca28-a388-c382-4b1a-4b65c9f9e6e7@google.com> The state/owner of FPU is saved fpu_fpregs_owner_ctx by pointing to the context that is currently loaded. It never changed during the life time of a task and remained stable/constant. Since deferred loading the FPU registers on return to userland, 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 gcc-9 is able to cache that load in copy_fpstate_to_sigframe() and reuse 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 of 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 defered FPU loading was added. Since this commit the compiler is no longer allowed 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. Link: https://bugzilla.kernel.org/show_bug.cgi?id=205663 Fixes: 5f409e20b7945 ("x86/fpu: Defer FPU state load until return to userspace") Debugged-by: Austin Clements <austin@google.com> Debugged-by: David Chase <drchase@golang.org> Debugged-by: Ian Lance Taylor <ian@airs.com> Tested-by: Borislav Petkov <bp@suse.de> Reviewed-by: Rik van Riel <riel@surriel.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- v1…v2: - Adding tags - Explaining why Fixes: does not point to the bisected commit. arch/x86/include/asm/fpu/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h index 4c95c365058aa..44c48e34d7994 100644 --- a/arch/x86/include/asm/fpu/internal.h +++ b/arch/x86/include/asm/fpu/internal.h @@ -509,7 +509,7 @@ static inline void __fpu_invalidate_fpregs_state(struct fpu *fpu) static inline int fpregs_state_valid(struct fpu *fpu, unsigned int cpu) { - return fpu == this_cpu_read_stable(fpu_fpregs_owner_ctx) && cpu == fpu->last_cpu; + return fpu == this_cpu_read(fpu_fpregs_owner_ctx) && cpu == fpu->last_cpu; } /* -- 2.24.0
next prev parent reply index Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-26 19:49 AVX register corruption from signal delivery Barret Rhoden 2019-11-26 20:20 ` Sebastian Andrzej Siewior 2019-11-26 21:23 ` Barret Rhoden 2019-11-26 22:13 ` Borislav Petkov 2019-11-26 22:30 ` Andy Lutomirski 2019-11-26 23:00 ` Borislav Petkov 2019-11-27 12:42 ` [PATCH] x86/fpu: Don't cache access to fpu_fpregs_owner_ctx Sebastian Andrzej Siewior 2019-11-27 14:07 ` Borislav Petkov 2019-11-27 18:42 ` Barret Rhoden 2019-11-28 8:53 ` Sebastian Andrzej Siewior [this message] 2019-11-28 9:22 ` [tip: x86/urgent] " tip-bot2 for Sebastian Andrzej Siewior 2019-11-29 16:57 ` [PATCH v2] " David Laight 2019-11-29 17:08 ` 'Sebastian Andrzej Siewior' 2019-11-27 15:46 ` [PATCH] " Rik van Riel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191128085306.hxfa2o3knqtu4wfn@linutronix.de \ --to=bigeasy@linutronix.de \ --cc=austin@google.com \ --cc=bp@alien8.de \ --cc=brho@google.com \ --cc=drchase@golang.org \ --cc=ian@airs.com \ --cc=josharian@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@redhat.com \ --cc=riel@surriel.com \ --cc=tglx@linutronix.de \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
LKML Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \ linux-kernel@vger.kernel.org public-inbox-index lkml Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git