[PATCH 04/10] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

[PATCH 04/10] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

[Yu-cheng Yu]

XSAVES is a kernel instruction and uses a compacted format. When
working with user space, the kernel should provide standard-format,
non-supervisor state data. We cannot do __copy_to_user() from a compacted-
format kernel xstate area to a signal frame.

Note that the path to copy_fpstate_to_sigframe() does currently check if
the thread has used FPU, but add a WARN_ONCE() there to detect any
potential mis-use.

Dave Hansen proposes this method to simplify copy xstate directly to user.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/kernel/fpu/signal.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 0fbf60c..7676718 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -130,6 +130,45 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
 	return err;
 }
 
+static int should_save_registers_directly(void)
+{
+	/*
+	 * In signal handling path, the kernel already checks if
+	 * FPU instructions have been used before it calls
+	 * copy_fpstate_to_sigframe(). We check this here again
+	 * to detect any potential mis-use and saving invalid
+	 * register values directly to a signal frame.
+	 */
+	WARN_ONCE(!current->thread.fpu.fpstate_active,
+		  "direct FPU save with no math use\n");
+
+	/*
+	 * In the case that we are using a compacted kernel
+	 * xsave area, we can not copy the thread.fpu.state
+	 * directly to userspace and *must* save it from the
+	 * registers directly.
+	 */
+	if (boot_cpu_has(X86_FEATURE_XSAVES))
+		return 1;
+
+	/*
+	 * fpregs_active() means "Can I use the FPU hardware
+	 * without taking a device-not-available exception?" This
+	 * means that saving the registers directly will be
+	 * cheaper than copying their contents out of
+	 * thread.fpu.state.
+	 *
+	 * Note that fpregs_active() is inherently racy and may
+	 * become false at any time.  If this race happens, we
+	 * will take a harmless device-not-available exception
+	 * when we attempt the FPU save instruction.
+	 */
+	if (fpregs_active())
+		return 1;
+
+	return 0;
+}
+
 /*
  * Save the fpu, extended register state to the user signal frame.
  *
@@ -167,7 +206,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 			sizeof(struct user_i387_ia32_struct), NULL,
 			(struct _fpstate_32 __user *) buf) ? -1 : 1;
 
-	if (fpregs_active()) {
+	if (should_save_registers_directly()) {
 		/* Save the live register state to the user directly. */
 		if (copy_fpregs_to_sigframe(buf_fx))
 			return -1;
-- 
1.9.1

Re: [PATCH 04/10] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

[Andy Lutomirski]

On Mon, Feb 22, 2016 at 10:58 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> XSAVES is a kernel instruction and uses a compacted format. When
> working with user space, the kernel should provide standard-format,
> non-supervisor state data. We cannot do __copy_to_user() from a compacted-
> format kernel xstate area to a signal frame.
>
> Note that the path to copy_fpstate_to_sigframe() does currently check if
> the thread has used FPU, but add a WARN_ONCE() there to detect any
> potential mis-use.
>
> Dave Hansen proposes this method to simplify copy xstate directly to user.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
>  arch/x86/kernel/fpu/signal.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 0fbf60c..7676718 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -130,6 +130,45 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
>         return err;
>  }
>
> +static int should_save_registers_directly(void)

I don't like the name of this function because:

> +{
> +       /*
> +        * In signal handling path, the kernel already checks if
> +        * FPU instructions have been used before it calls
> +        * copy_fpstate_to_sigframe(). We check this here again
> +        * to detect any potential mis-use and saving invalid
> +        * register values directly to a signal frame.
> +        */
> +       WARN_ONCE(!current->thread.fpu.fpstate_active,
> +                 "direct FPU save with no math use\n");

... Here "direct" seems to mean that we're asking whether to directly save ...

> +
> +       /*
> +        * In the case that we are using a compacted kernel
> +        * xsave area, we can not copy the thread.fpu.state
> +        * directly to userspace and *must* save it from the
> +        * registers directly.
> +        */

... and here "directly" means *both* copying directly to userspace and
saving using xsave directly.

So can you rename it to something with an obvious meaning like
"may_memcpy_fpu_regs" or similar?

--Andy

Re: [PATCH 04/10] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

[Yu-cheng Yu]

On Mon, Feb 22, 2016 at 02:42:54PM -0800, Andy Lutomirski wrote:
> > +static int should_save_registers_directly(void)
> 
> I don't like the name of this function because:
> 
> > +{
> > +       /*
> > +        * In signal handling path, the kernel already checks if
> > +        * FPU instructions have been used before it calls
> > +        * copy_fpstate_to_sigframe(). We check this here again
> > +        * to detect any potential mis-use and saving invalid
> > +        * register values directly to a signal frame.
> > +        */
> > +       WARN_ONCE(!current->thread.fpu.fpstate_active,
> > +                 "direct FPU save with no math use\n");
> 
> ... Here "direct" seems to mean that we're asking whether to directly save ...
> 
> > +
> > +       /*
> > +        * In the case that we are using a compacted kernel
> > +        * xsave area, we can not copy the thread.fpu.state
> > +        * directly to userspace and *must* save it from the
> > +        * registers directly.
> > +        */
> 
> ... and here "directly" means *both* copying directly to userspace and
> saving using xsave directly.
> 
> So can you rename it to something with an obvious meaning like
> "may_memcpy_fpu_regs" or similar?

This function determines whether the kernel should do
copy_fpregs_to_sigframe(). Perhaps we can re-name it to
may_copy_fpregs_to_sigframe()?

--Yu-cheng

Re: [PATCH 04/10] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

[Andy Lutomirski]

On Mon, Feb 22, 2016 at 3:27 PM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> On Mon, Feb 22, 2016 at 02:42:54PM -0800, Andy Lutomirski wrote:
>> > +static int should_save_registers_directly(void)
>>
>> I don't like the name of this function because:
>>
>> > +{
>> > +       /*
>> > +        * In signal handling path, the kernel already checks if
>> > +        * FPU instructions have been used before it calls
>> > +        * copy_fpstate_to_sigframe(). We check this here again
>> > +        * to detect any potential mis-use and saving invalid
>> > +        * register values directly to a signal frame.
>> > +        */
>> > +       WARN_ONCE(!current->thread.fpu.fpstate_active,
>> > +                 "direct FPU save with no math use\n");
>>
>> ... Here "direct" seems to mean that we're asking whether to directly save ...
>>
>> > +
>> > +       /*
>> > +        * In the case that we are using a compacted kernel
>> > +        * xsave area, we can not copy the thread.fpu.state
>> > +        * directly to userspace and *must* save it from the
>> > +        * registers directly.
>> > +        */
>>
>> ... and here "directly" means *both* copying directly to userspace and
>> saving using xsave directly.
>>
>> So can you rename it to something with an obvious meaning like
>> "may_memcpy_fpu_regs" or similar?
>
> This function determines whether the kernel should do
> copy_fpregs_to_sigframe(). Perhaps we can re-name it to
> may_copy_fpregs_to_sigframe()?
>

Works for me.


> --Yu-cheng
>



-- 
Andy Lutomirski
AMA Capital Management, LLC