diff mbox series

[v5,06/39] x86/fpu: Add helper for modifying xstate

Message ID 20230119212317.8324-7-rick.p.edgecombe@intel.com
State New
Headers
Series Shadow stacks for userspace |

Commit Message

Edgecombe, Rick P Jan. 19, 2023, 9:22 p.m. UTC 
  Just like user xfeatures, supervisor xfeatures can be active in the
registers or present in the task FPU buffer. If the registers are
active, the registers can be modified directly. If the registers are
not active, the modification must be performed on the task FPU buffer.

When the state is not active, the kernel could perform modifications
directly to the buffer. But in order for it to do that, it needs
to know where in the buffer the specific state it wants to modify is
located. Doing this is not robust against optimizations that compact
the FPU buffer, as each access would require computing where in the
buffer it is.

The easiest way to modify supervisor xfeature data is to force restore
the registers and write directly to the MSRs. Often times this is just fine
anyway as the registers need to be restored before returning to userspace.
Do this for now, leaving buffer writing optimizations for the future.

Add a new function fpregs_lock_and_load() that can simultaneously call
fpregs_lock() and do this restore. Also perform some extra sanity
checks in this function since this will be used in non-fpu focused code.

Tested-by: Pengfei Xu <pengfei.xu@intel.com>
Tested-by: John Allen <john.allen@amd.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---

v5:
 - Fix spelling error (Boris)
 - Don't export fpregs_lock_and_load() (Boris)

v3:
 - Rename to fpregs_lock_and_load() to match the unlocking
   fpregs_unlock(). (Kees)
 - Elaborate in comment about helper. (Dave)

v2:
 - Drop optimization of writing directly the buffer, and change API
   accordingly.
 - fpregs_lock_and_load() suggested by tglx
 - Some commit log verbiage from dhansen

v1:
 - New patch.

 arch/x86/include/asm/fpu/api.h |  9 +++++++++
 arch/x86/kernel/fpu/core.c     | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+)

Comments

Kees Cook Jan. 20, 2023, 12:47 a.m. UTC | #1 
On Thu, Jan 19, 2023 at 01:22:44PM -0800, Rick Edgecombe wrote:
> Just like user xfeatures, supervisor xfeatures can be active in the
> registers or present in the task FPU buffer. If the registers are
> active, the registers can be modified directly. If the registers are
> not active, the modification must be performed on the task FPU buffer.
> 
> When the state is not active, the kernel could perform modifications
> directly to the buffer. But in order for it to do that, it needs
> to know where in the buffer the specific state it wants to modify is
> located. Doing this is not robust against optimizations that compact
> the FPU buffer, as each access would require computing where in the
> buffer it is.
> 
> The easiest way to modify supervisor xfeature data is to force restore
> the registers and write directly to the MSRs. Often times this is just fine
> anyway as the registers need to be restored before returning to userspace.
> Do this for now, leaving buffer writing optimizations for the future.
> 
> Add a new function fpregs_lock_and_load() that can simultaneously call
> fpregs_lock() and do this restore. Also perform some extra sanity
> checks in this function since this will be used in non-fpu focused code.
> 
> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> Tested-by: John Allen <john.allen@amd.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

Reviewed-by: Kees Cook <keescook@chromium.org>
Borislav Petkov Feb. 1, 2023, 11:01 a.m. UTC | #2 
On Thu, Jan 19, 2023 at 01:22:44PM -0800, Rick Edgecombe wrote:
> +void fpregs_lock_and_load(void)
> +{
> +	/*
> +	 * fpregs_lock() only disables preemption (mostly). So modifying state
> +	 * in an interrupt could screw up some in progress fpregs operation,
> +	 * but appear to work. Warn about it.

I don't like comments where it sounds like we don't know what we're
doing. "Appear to work"?
Edgecombe, Rick P Feb. 1, 2023, 5:31 p.m. UTC | #3 
On Wed, 2023-02-01 at 12:01 +0100, Borislav Petkov wrote:
> On Thu, Jan 19, 2023 at 01:22:44PM -0800, Rick Edgecombe wrote:
> > +void fpregs_lock_and_load(void)
> > +{
> > +     /*
> > +      * fpregs_lock() only disables preemption (mostly). So
> > modifying state
> > +      * in an interrupt could screw up some in progress fpregs
> > operation,
> > +      * but appear to work. Warn about it.
> 
> I don't like comments where it sounds like we don't know what we're
> doing. "Appear to work"?

I can change it. This patch started with the observation that modifying
xstate from the kernel had been gotten wrong a couple times in the
past, so that is what this is referencing. Since then, the fancy
automatic solution got boiled down to this helper and a couple
warnings.
Borislav Petkov Feb. 1, 2023, 6:18 p.m. UTC | #4 
On Wed, Feb 01, 2023 at 05:31:50PM +0000, Edgecombe, Rick P wrote:
> On Wed, 2023-02-01 at 12:01 +0100, Borislav Petkov wrote:
> > On Thu, Jan 19, 2023 at 01:22:44PM -0800, Rick Edgecombe wrote:
> > > +void fpregs_lock_and_load(void)
> > > +{
> > > +     /*
> > > +      * fpregs_lock() only disables preemption (mostly). So
> > > modifying state
> > > +      * in an interrupt could screw up some in progress fpregs
> > > operation,
> > > +      * but appear to work. Warn about it.
> > 
> > I don't like comments where it sounds like we don't know what we're
> > doing. "Appear to work"?
> 
> I can change it. This patch started with the observation that modifying
> xstate from the kernel had been gotten wrong a couple times in the
> past, so that is what this is referencing. Since then, the fancy
> automatic solution got boiled down to this helper and a couple
> warnings.

Yeah, but that comment right now reads like: modifying in interrupt
context can corrupt fpregs and you should not do it but it kinda works,
by chance. Thus encouraging people to keep doing that.

I guess "but appear to work" can go and then it is fine.

Thx.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index 503a577814b2..aadc6893dcaa 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -82,6 +82,15 @@  static inline void fpregs_unlock(void)
 		preempt_enable();
 }
 
+/*
+ * FPU state gets lazily restored before returning to userspace. So when in the
+ * kernel, the valid FPU state may be kept in the buffer. This function will force
+ * restore all the fpu state to the registers early if needed, and lock them from
+ * being automatically saved/restored. Then FPU state can be modified safely in the
+ * registers, before unlocking with fpregs_unlock().
+ */
+void fpregs_lock_and_load(void);
+
 #ifdef CONFIG_X86_DEBUG_FPU
 extern void fpregs_assert_state_consistent(void);
 #else
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index dccce58201b7..7317bfd5ea36 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -753,6 +753,24 @@  void switch_fpu_return(void)
 }
 EXPORT_SYMBOL_GPL(switch_fpu_return);
 
+void fpregs_lock_and_load(void)
+{
+	/*
+	 * fpregs_lock() only disables preemption (mostly). So modifying state
+	 * in an interrupt could screw up some in progress fpregs operation,
+	 * but appear to work. Warn about it.
+	 */
+	WARN_ON_ONCE(!irq_fpu_usable());
+	WARN_ON_ONCE(current->flags & PF_KTHREAD);
+
+	fpregs_lock();
+
+	fpregs_assert_state_consistent();
+
+	if (test_thread_flag(TIF_NEED_FPU_LOAD))
+		fpregs_restore_userregs();
+}
+
 #ifdef CONFIG_X86_DEBUG_FPU
 /*
  * If current FPU state according to its tracking (loaded FPU context on this