Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1567081 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] x86/fpu: copyout_from_xsaves & copyin_to_xsaves fixes riel@redhat.com - 2017-01-26 03:10 +0100
    [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state riel@redhat.com - 2017-01-26 03:10 +0100
      Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Ingo Molnar <mingo@kernel.org> - 2017-01-26 09:20 +0100
        [PATCH] x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves() to  copy_user_to_xstate()/copy_xstate_to_user() Ingo Molnar <mingo@kernel.org> - 2017-01-26 09:30 +0100
          Re: [PATCH] x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves()  to copy_user_to_xstate()/copy_xstate_to_user() Ingo Molnar <mingo@kernel.org> - 2017-01-26 11:30 +0100
      Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Dave Hansen <dave.hansen@linux.intel.com> - 2017-01-26 16:10 +0100
      Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Yu-cheng Yu <yu-cheng.yu@intel.com> - 2017-01-30 18:40 +0100

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

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.

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

* 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

[toc] | [prev] | [next] | [standalone]

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

* 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;

[toc] | [prev] | [next] | [standalone]

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

* 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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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
 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web