[PATCH 0/2] x86/fpu: copyout_from_xsaves & copyin_to_xsaves fixes

[PATCH 0/2] x86/fpu: copyout_from_xsaves & copyin_to_xsaves fixes

[riel]

There are two issues with copyout_from_xsaves and copyin_to_xsaves.

The first is a simple bounds checking issue, where the code could
potentially clobber memory outside of a userspace buffer before it
stops copying data.

The second is more subtle. SSE and YMM XRSTOR depend on two fields
inside the legacy FP area. However, if xfeatures XFEATURE_MASK_FP is
clear, those fields do not get copied around at all. Fix that.

Thanks to Dave Hansen for helping track down that second bug.

[PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy

[riel]

From: Rik van Riel <riel@redhat.com>

Userspace may have programs, especially debuggers, that do not know
how large the full XSAVE area space is. They pass in a size argument,
and expect to not get more data than that.

Unfortunately, the current copyout_from_xsaves does the bounds check
after the copy out to userspace.  This could theoretically result
in the kernel scribbling over userspace memory outside of the buffer,
before bailing out of the copy.

In practice, this is not likely to be an issue, since debuggers are
likely to specify the size they know about, and that size is likely
to exactly match the XSAVE fields they know about.

However, we could be a little more careful and do the bounds check
before committing ourselves with a copy to userspace.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/kernel/fpu/xstate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c24ac1efb12d..c1508d56ecfb 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -992,13 +992,13 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
 			offset = xstate_offsets[i];
 			size = xstate_sizes[i];
 
+			if (offset + size > count)
+				break;
+
 			ret = xstate_copyout(offset, size, kbuf, ubuf, src, 0, count);
 
 			if (ret)
 				return ret;
-
-			if (offset + size >= count)
-				break;
 		}
 
 	}
-- 
2.9.3

Re: [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy

[Ingo Molnar]

* riel@redhat.com <riel@redhat.com> wrote:

> From: Rik van Riel <riel@redhat.com>
> 
> Userspace may have programs, especially debuggers, that do not know
> how large the full XSAVE area space is. They pass in a size argument,
> and expect to not get more data than that.
> 
> Unfortunately, the current copyout_from_xsaves does the bounds check
> after the copy out to userspace.  This could theoretically result
> in the kernel scribbling over userspace memory outside of the buffer,
> before bailing out of the copy.
> 
> In practice, this is not likely to be an issue, since debuggers are
> likely to specify the size they know about, and that size is likely
> to exactly match the XSAVE fields they know about.
> 
> However, we could be a little more careful and do the bounds check
> before committing ourselves with a copy to userspace.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> ---
>  arch/x86/kernel/fpu/xstate.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c24ac1efb12d..c1508d56ecfb 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -992,13 +992,13 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
>  			offset = xstate_offsets[i];
>  			size = xstate_sizes[i];
>  
> +			if (offset + size > count)
> +				break;
> +
>  			ret = xstate_copyout(offset, size, kbuf, ubuf, src, 0, count);
>  
>  			if (ret)
>  				return ret;
> -
> -			if (offset + size >= count)
> -				break;

That's not a robust way to do a bounds check either - what if 'offset' is so large 
that it overflows and offset + size falls within the 'sane' 0..count range?

Also, what about partial copies?

Plus, to add insult to injury, xstate_copyout() is a totally unnecessary 
obfuscation to begin with:

 - 'start_pos' is always 0

 - 'end_pos' is always 'count'

 - both are const for no good reason: they are not pointers

 - both are signed for no good reason: they are derived from unsigned types and I 
   don't see how negative values can ever be valid here.

So this code:

static inline int xstate_copyout(unsigned int pos, unsigned int count,
                                 void *kbuf, void __user *ubuf,
                                 const void *data, const int start_pos,
                                 const int end_pos)
{
        if ((count == 0) || (pos < start_pos))
                return 0;

        if (end_pos < 0 || pos < end_pos) {
                unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - pos));

                if (kbuf) {
                        memcpy(kbuf + pos, data, copy);
                } else {
                        if (__copy_to_user(ubuf + pos, data, copy))
                                return -EFAULT;
                }
        }
        return 0;
}

Is, after all the cleanups and fixes is in reality equivalent to:

static inline int
__copy_xstate_to_kernel(void *kbuf, const void *data,
                        unsigned int offset, unsigned int size)
{
        memcpy(kbuf + offset, data, size);

        return 0;
}

!!!

So the real fix is to get rid of xstate_copyout() altogether and just do the 
memcpy directly - the regset obfuscation actively hid a real bug...

Thanks,

	Ingo

Re: [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> So this code:
> 
> static inline int xstate_copyout(unsigned int pos, unsigned int count,
>                                  void *kbuf, void __user *ubuf,
>                                  const void *data, const int start_pos,
>                                  const int end_pos)
> {
>         if ((count == 0) || (pos < start_pos))
>                 return 0;
> 
>         if (end_pos < 0 || pos < end_pos) {
>                 unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - pos));
> 
>                 if (kbuf) {
>                         memcpy(kbuf + pos, data, copy);
>                 } else {
>                         if (__copy_to_user(ubuf + pos, data, copy))
>                                 return -EFAULT;
>                 }
>         }
>         return 0;
> }
> 
> Is, after all the cleanups and fixes is in reality equivalent to:
> 
> static inline int
> __copy_xstate_to_kernel(void *kbuf, const void *data,
>                         unsigned int offset, unsigned int size)
> {
>         memcpy(kbuf + offset, data, size);
> 
>         return 0;
> }
> 
> !!!

Note that it's not entirely true - for the degenerate case of ptrace() requesting 
a very small and partial buffer that cannot even fit the headers, this check is 
still required - so we end up with something like:

static inline int
__copy_xstate_to_kernel(void *kbuf, const void *data,
                        unsigned int offset, unsigned int size, unsigned int size_total)
{ 
        if (offset < size_total) {
                unsigned int copy = min(size, size_total - offset);
                
                memcpy(kbuf + offset, data, copy);
        }       
        return 0;
}       

But it's still an inconsistent mess: we'll do a partial copy in headers but not 
for xstate components?

I believe the right solution is to allow partial copies only if they are at 
precise xstate (and legacy) component boundaries, and apply this to the header 
portion as well.

This allows user-space to request only the FPU bits for example - but doesn't 
force the kernel to handle really weird partial copy cases that very few people 
are testing ...

(Unless there's some ABI pattern from debugging applications that I missed?)

Thanks,

	Ingo

[PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state

[riel]

From: Rik van Riel <riel@redhat.com>

On Skylake CPUs I noticed that XRSTOR is unable to deal with states
created by copyout_from_xsaves if the xstate has only SSE/YMM state, and
no FP state. That is, xfeatures had XFEATURE_MASK_SSE set, but not
XFEATURE_MASK_FP.

The reason is that part of the SSE/YMM state lives in the MXCSR and
MXCSR_FLAGS fields of the FP state.

Ensure that whenever we copy SSE or YMM state around, the MXCSR and
MXCSR_FLAGS fields are also copied around.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/kernel/fpu/xstate.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c1508d56ecfb..10b10917af81 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1004,6 +1004,23 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
 	}
 
 	/*
+	 * Restoring SSE/YMM state requires that MXCSR & MXCSR_MASK are saved.
+	 * Those fields are part of the legacy FP state, and only get saved
+	 * above if XFEATURES_MASK_FP is set.
+	 *
+	 * Copy out those fields if we have SSE/YMM but no FP register data.
+	 */
+	if ((header.xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) &&
+			!(header.xfeatures & XFEATURE_MASK_FP)) {
+		size = sizeof(u64);
+		ret = xstate_copyout(offset, size, kbuf, ubuf,
+				     &xsave->i387.mxcsr, 0, count);
+
+		if (ret)
+			return ret;
+	}
+
+	/*
 	 * Fill xsave->i387.sw_reserved value for ptrace frame:
 	 */
 	offset = offsetof(struct fxregs_state, sw_reserved);
@@ -1030,6 +1047,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 	int i;
 	u64 xfeatures;
 	u64 allowed_features;
+	void *dst;
 
 	offset = offsetof(struct xregs_state, header);
 	size = sizeof(xfeatures);
@@ -1053,7 +1071,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 		u64 mask = ((u64)1 << i);
 
 		if (xfeatures & mask) {
-			void *dst = __raw_xsave_addr(xsave, 1 << i);
+			dst = __raw_xsave_addr(xsave, 1 << i);
 
 			offset = xstate_offsets[i];
 			size = xstate_sizes[i];
@@ -1068,6 +1086,25 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 	}
 
 	/*
+	 * SSE/YMM state depends on the MXCSR & MXCSR_MASK fields from the FP
+	 * state. If we restored only SSE/YMM state but not FP state, copy
+	 * those fields to ensure the SSE/YMM state restore works.
+	 */
+	if ((xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) &&
+			!(xfeatures & XFEATURE_MASK_FP)) {
+		offset = offsetof(struct fxregs_state, mxcsr);
+		dst = xsave + offset;
+		size = sizeof(u64);
+
+		if (kbuf) {
+			memcpy(dst, kbuf + offset, size);
+		} else {
+			if (__copy_from_user(dst, ubuf + offset, size))
+				return -EFAULT;
+		}
+	}
+
+	/*
 	 * The state that came in from userspace was user-state only.
 	 * Mask all the user states out of 'xfeatures':
 	 */
-- 
2.9.3

Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state

[Ingo Molnar]

* riel@redhat.com <riel@redhat.com> wrote:

> From: Rik van Riel <riel@redhat.com>
> 
> On Skylake CPUs I noticed that XRSTOR is unable to deal with states
> created by copyout_from_xsaves if the xstate has only SSE/YMM state, and
> no FP state. That is, xfeatures had XFEATURE_MASK_SSE set, but not
> XFEATURE_MASK_FP.
> 
> The reason is that part of the SSE/YMM state lives in the MXCSR and
> MXCSR_FLAGS fields of the FP state.
> 
> Ensure that whenever we copy SSE or YMM state around, the MXCSR and
> MXCSR_FLAGS fields are also copied around.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> ---
>  arch/x86/kernel/fpu/xstate.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c1508d56ecfb..10b10917af81 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1004,6 +1004,23 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
>  	}
>  
>  	/*
> +	 * Restoring SSE/YMM state requires that MXCSR & MXCSR_MASK are saved.
> +	 * Those fields are part of the legacy FP state, and only get saved
> +	 * above if XFEATURES_MASK_FP is set.
> +	 *
> +	 * Copy out those fields if we have SSE/YMM but no FP register data.
> +	 */
> +	if ((header.xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) &&
> +			!(header.xfeatures & XFEATURE_MASK_FP)) {
> +		size = sizeof(u64);
> +		ret = xstate_copyout(offset, size, kbuf, ubuf,
> +				     &xsave->i387.mxcsr, 0, count);

So this u64 copy copies both i387.mxcsr and i387.mxcsr_mask, which only works 
because the two fields are next to each other and there's no hole inbetween in the 
structure, right?

That fact should at minimum be commented upon.

> @@ -1053,7 +1071,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,

Also, I clearly wasn't paying enough attention when I merged the commit that 
introduced these ptrace conversion bits:

  91c3dba7dbc1 ("x86/fpu/xstate: Fix PTRACE frames for XSAVES")

1)

the 'copyin/copyout' nomenclature needlessly departs from what the modern FPU code 
uses, which is:

 copy_fpregs_to_fpstate()
 copy_fpstate_to_sigframe()
 copy_fregs_to_user()
 copy_fxregs_to_kernel()
 copy_fxregs_to_user()
 copy_kernel_to_fpregs()
 copy_kernel_to_fregs()
 copy_kernel_to_fxregs()
 copy_kernel_to_xregs()
 copy_user_to_fregs()
 copy_user_to_fxregs()
 copy_user_to_xregs()
 copy_xregs_to_kernel()
 copy_xregs_to_user()

I.e. according to this pattern, the following rename should be done:

  copyin_to_xsaves()    -> copy_user_to_xstate()
  copyout_from_xsaves() -> copy_xstate_to_user()

or, if we want to be pedantic, denote that that the user-space format is ptrace:

  copyin_to_xsaves()    -> copy_user_ptrace_to_xstate()
  copyout_from_xsaves() -> copy_xstate_to_user_ptrace()

(But I'd suggest the shorter, non-pedantic name.)

But there's other problems:

2)

The copy_user_to_xstate() parameter order departs from the regular memcpy() 
pattern we try to follow:

  int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
                     struct xregs_state *xsave);

it should be the other way around:

  int copy_user_to_xstate(struct xregs_state *xsave, const void *kbuf, const void __user *ubuf)

3)

But there's worse problems - the 'kbuf' parameter in both APIs, for example in 
copy_xstate_to_user():

        if (kbuf) {
                memcpy(&xfeatures, kbuf + offset, size);
        } else {
                if (__copy_from_user(&xfeatures, ubuf + offset, size))
                        return -EFAULT;
	}

WTF: memory copy API semantics dependent on argument presence? Whether it's truly 
a 'user' copy depends on whether 'kbuf' is NULL??

This should be split into four APIs:

	copy_xstate_to_user()
	copy_xstate_to_kernel()
	copy_user_to_xstate()
	copy_kernel_to_xstate()

This decoupling would remove the weird 'kbuf, ubuf, xstate' triple argument 
dependence and turn them into regular two-argument memcpy() variant APIs:

	copy_xstate_to_user   (ubuf, xstate)
	copy_xstate_to_kernel (kbuf, xstate)
	copy_user_to_xstate   (xstate, ubuf)
	copy_kernel_to_xstate (xstate, kbuf)

... and would restore the type cleanliness/robustness of these APIs as well.

4)

>  	/*
> +	 * SSE/YMM state depends on the MXCSR & MXCSR_MASK fields from the FP
> +	 * state. If we restored only SSE/YMM state but not FP state, copy
> +	 * those fields to ensure the SSE/YMM state restore works.
> +	 */
> +	if ((xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) &&
> +			!(xfeatures & XFEATURE_MASK_FP)) {

So this pattern is used twice and it's quite a mouthful. How about introducing 
such a helper:

/*
 * Weird legacy quirk: indicate whether the MXCSR/MXCSR_MASK part of the FP state 
 * is used, even though the xfeatures flag lies about it being unused:
 */
static inline bool xfeatures_fp_mxcsr_used(u64 xfeatures)
{
	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
		return 0;

	if (xfeatures & XFEATURE_MASK_FP)
		return 0;

	return 1;
}

?

5)

While at it I noticed this code:

                u64 mask = ((u64)1 << i);

instead of the ugly type cast, cannot that be written as:

		u64 mask = 1ULL << i;

which is shorter and cleaner?

I.e. this code needs some serious love and I'm not surprised it had bugs in it...

But hindsight is 20/20 and I merged it myself and all that, so I'm not really 
complaining - but let's not repeat the mistake, ok?

Thanks,

	Ingo

[PATCH] x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves() to copy_user_to_xstate()/copy_xstate_to_user()

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 1)
> 
> the 'copyin/copyout' nomenclature needlessly departs from what the modern FPU code 
> uses, which is:

The patch below implements this first step. Untested.

Thanks,

	Ingo

====>
>From c9459f7130a33c9d0108ca1f93306fb71772038f Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Thu, 26 Jan 2017 09:17:46 +0100
Subject: [PATCH] x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves() to copy_user_to_xstate()/copy_xstate_to_user()

The 'copyin/copyout' nomenclature needlessly departs from what the modern FPU code
uses, which is:

 copy_fpregs_to_fpstate()
 copy_fpstate_to_sigframe()
 copy_fregs_to_user()
 copy_fxregs_to_kernel()
 copy_fxregs_to_user()
 copy_kernel_to_fpregs()
 copy_kernel_to_fregs()
 copy_kernel_to_fxregs()
 copy_kernel_to_xregs()
 copy_user_to_fregs()
 copy_user_to_fxregs()
 copy_user_to_xregs()
 copy_xregs_to_kernel()
 copy_xregs_to_user()

I.e. according to this pattern, the following rename should be done:

  copyin_to_xsaves()    -> copy_user_to_xstate()
  copyout_from_xsaves() -> copy_xstate_to_user()

or, if we want to be pedantic, denote that that the user-space format is ptrace:

  copyin_to_xsaves()    -> copy_user_ptrace_to_xstate()
  copyout_from_xsaves() -> copy_xstate_to_user_ptrace()

But I'd suggest the shorter, non-pedantic name.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/xstate.h | 4 ++--
 arch/x86/kernel/fpu/regset.c      | 4 ++--
 arch/x86/kernel/fpu/signal.c      | 2 +-
 arch/x86/kernel/fpu/xstate.c      | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 1b2799e0699a..a1baa17e9748 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -48,8 +48,8 @@ void fpu__xstate_clear_all_cpu_caps(void);
 void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
-int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
+int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
 			void __user *ubuf, struct xregs_state *xsave);
-int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
+int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
 		     struct xregs_state *xsave);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index c114b132d121..4efb81d6ee74 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -91,7 +91,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
 	fpu__activate_fpstate_read(fpu);
 
 	if (using_compacted_format()) {
-		ret = copyout_from_xsaves(pos, count, kbuf, ubuf, xsave);
+		ret = copy_xstate_to_user(pos, count, kbuf, ubuf, xsave);
 	} else {
 		fpstate_sanitize_xstate(fpu);
 		/*
@@ -131,7 +131,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 	fpu__activate_fpstate_write(fpu);
 
 	if (boot_cpu_has(X86_FEATURE_XSAVES))
-		ret = copyin_to_xsaves(kbuf, ubuf, xsave);
+		ret = copy_user_to_xstate(kbuf, ubuf, xsave);
 	else
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
 
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 83c23c230b4c..b1fe9a1fc4e0 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -324,7 +324,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		fpu__drop(fpu);
 
 		if (using_compacted_format()) {
-			err = copyin_to_xsaves(NULL, buf_fx,
+			err = copy_user_to_xstate(NULL, buf_fx,
 					       &fpu->state.xsave);
 		} else {
 			err = __copy_from_user(&fpu->state.xsave,
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c24ac1efb12d..e7bb41723eaa 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -951,7 +951,7 @@ static inline int xstate_copyout(unsigned int pos, unsigned int count,
  * zero. This is called from xstateregs_get() and there we check the CPU
  * has XSAVES.
  */
-int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
+int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
 			void __user *ubuf, struct xregs_state *xsave)
 {
 	unsigned int offset, size;
@@ -1023,7 +1023,7 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
  * there we check the CPU has XSAVES and a whole standard-sized buffer
  * exists.
  */
-int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
+int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
 		     struct xregs_state *xsave)
 {
 	unsigned int offset, size;

Re: [PATCH] x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves() to copy_user_to_xstate()/copy_xstate_to_user()

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 1)
> > 
> > the 'copyin/copyout' nomenclature needlessly departs from what the modern FPU code 
> > uses, which is:
> 
> The patch below implements this first step. Untested.

I sent out the other cleanups as well, in:

  [PATCH 00/14] x86/fpu: Clean up ptrace copying functions

which includes the first fix, and if it all works, could be used as a base for the 
second fix as well.

Thanks,

	Ingo

Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state

[Dave Hansen]

On 01/25/2017 05:57 PM, riel@redhat.com wrote:
>  	/*
> +	 * Restoring SSE/YMM state requires that MXCSR & MXCSR_MASK are saved.
> +	 * Those fields are part of the legacy FP state, and only get saved
> +	 * above if XFEATURES_MASK_FP is set.
> +	 *
> +	 * Copy out those fields if we have SSE/YMM but no FP register data.
> +	 */

Patch looks functionally good to me.  One nit on the comment, though:
Usually XSAVE "state" refers to the state in the CPU while the "area"
refers to the buffer in memory.  Probably best to talk about the "legacy
FP area", not state.

Maybe something like this for the first paragraph:

	MXCSR and MXCSR_MASK are part of the SSE and YMM states and are
	saved/restored along with those states.  However, they are
	located in the legacy FP *area* of the XSAVE buffer.

Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state

[Yu-cheng Yu]

On Wed, Jan 25, 2017 at 08:57:59PM -0500, riel@redhat.com wrote:
> From: Rik van Riel <riel@redhat.com>
> 
> On Skylake CPUs I noticed that XRSTOR is unable to deal with states
> created by copyout_from_xsaves if the xstate has only SSE/YMM state, and
> no FP state. That is, xfeatures had XFEATURE_MASK_SSE set, but not
> XFEATURE_MASK_FP.
> 
> The reason is that part of the SSE/YMM state lives in the MXCSR and
> MXCSR_FLAGS fields of the FP state.
> 
> Ensure that whenever we copy SSE or YMM state around, the MXCSR and
> MXCSR_FLAGS fields are also copied around.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> ---
>  arch/x86/kernel/fpu/xstate.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c1508d56ecfb..10b10917af81 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1004,6 +1004,23 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
>  	}
>  
>  	/*
> +	 * Restoring SSE/YMM state requires that MXCSR & MXCSR_MASK are saved.
> +	 * Those fields are part of the legacy FP state, and only get saved
> +	 * above if XFEATURES_MASK_FP is set.
> +	 *
> +	 * Copy out those fields if we have SSE/YMM but no FP register data.
> +	 */
> +	if ((header.xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) &&
> +			!(header.xfeatures & XFEATURE_MASK_FP)) {
> +		size = sizeof(u64);
> +		ret = xstate_copyout(offset, size, kbuf, ubuf,
> +				     &xsave->i387.mxcsr, 0, count);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
>  	 * Fill xsave->i387.sw_reserved value for ptrace frame:
>  	 */
>  	offset = offsetof(struct fxregs_state, sw_reserved);
> @@ -1030,6 +1047,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
>  	int i;
>  	u64 xfeatures;
>  	u64 allowed_features;
> +	void *dst;
>  
>  	offset = offsetof(struct xregs_state, header);
>  	size = sizeof(xfeatures);
> @@ -1053,7 +1071,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
>  		u64 mask = ((u64)1 << i);
>  
>  		if (xfeatures & mask) {
> -			void *dst = __raw_xsave_addr(xsave, 1 << i);
> +			dst = __raw_xsave_addr(xsave, 1 << i);
>  
>  			offset = xstate_offsets[i];
>  			size = xstate_sizes[i];
> @@ -1068,6 +1086,25 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
>  	}
>  
>  	/*
> +	 * SSE/YMM state depends on the MXCSR & MXCSR_MASK fields from the FP
> +	 * state. If we restored only SSE/YMM state but not FP state, copy
> +	 * those fields to ensure the SSE/YMM state restore works.
> +	 */

In xstateregs_set(), we enforced the starting pos must be from (0), which in
XSAVE time, was probably for this reason.  The real mistake here, I think, is
allowing skipping of xstate[0] and xstate[1].  Both should have been there
even for XSAVES compacted-format.  Would it be a simpler fix just making sure
xstate[0] and xstate[1] are copied?

Yu-cheng