[patch] x86, i387: use convert_to_fxsr() in fpregs_set()

[patch] x86, i387: use convert_to_fxsr() in fpregs_set()

[Siddha, Suresh B]

Roland, Just happen to notice this bug. Can you please ack the bug fix which
needs to goto x86 mm tree.

thanks.
---
[patch] x86, i387: use convert_to_fxsr() in fpregs_set()

This fixes the bug introduced recently during the revamp of the code.
fpregs_set() need to use convert_to_fxsr() rather than copying into the
fxsave struct directly.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 7e354a3..93a1706 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -327,6 +327,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 	       const void *kbuf, const void __user *ubuf)
 {
 	int ret;
+	struct user_i387_ia32_struct env;
 
 	if (!HAVE_HWFP)
 		return fpregs_soft_set(target, regset, pos, count, kbuf, ubuf);
@@ -339,13 +340,9 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 					  &target->thread.i387.fsave, 0, -1);
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-				 &target->thread.i387.fxsave, 0, -1);
-
-	/*
-	 * mxcsr reserved bits must be masked to zero for security reasons.
-	 */
-	target->thread.i387.fxsave.mxcsr &= mxcsr_feature_mask;
+				 &env, 0, -1);
 
+	convert_to_fxsr(target, &env);
 	return ret;
 }
 

[PATCH x86/mm] x86: i387 fpregs_set convert_to_fxsr

[Roland McGrath]

Thanks for catching that, Suresh.  
The fix needed a few nits different from your patch.


Thanks,
Roland

---

This fixes the bug introduced recently during the revamp of the code.
fpregs_set() needs to use convert_to_fxsr() rather than copying into the
fxsave struct directly.

Reported-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/x86/kernel/i387.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 7e354a3..26719bd 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -326,6 +326,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 	       unsigned int pos, unsigned int count,
 	       const void *kbuf, const void __user *ubuf)
 {
+	struct user_i387_ia32_struct env;
 	int ret;
 
 	if (!HAVE_HWFP)
@@ -338,13 +339,12 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 		return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
 					  &target->thread.i387.fsave, 0, -1);
 
-	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-				 &target->thread.i387.fxsave, 0, -1);
+	if (pos > 0 || count < sizeof(env))
+		convert_from_fxsr(&env, target);
 
-	/*
-	 * mxcsr reserved bits must be masked to zero for security reasons.
-	 */
-	target->thread.i387.fxsave.mxcsr &= mxcsr_feature_mask;
+	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &env, 0, -1);
+	if (!ret)
+		convert_to_fxsr(target, &env);
 
 	return ret;
 }

Re: [patch] x86, i387: use convert_to_fxsr() in fpregs_set()

[Ingo Molnar]

* Siddha, Suresh B <suresh.b.siddha@intel.com> wrote:

> Roland, Just happen to notice this bug. Can you please ack the bug fix 
> which needs to goto x86 mm tree.

nice one! I'm wondering, how did you notice this bug? Did something 
crash? Did FPU state get corrupted?

(i've applied Roland's followup patch to x86.git)

	Ingo

Re: [PATCH x86/mm] x86: i387 fpregs_set convert_to_fxsr

[Siddha, Suresh B]

On Thu, Jan 24, 2008 at 05:59:33PM -0800, Roland McGrath wrote:
> +	if (pos > 0 || count < sizeof(env))
> +		convert_from_fxsr(&env, target);

Well, is the generic regset code enforce the need for this now? Can we
disallow the usage cases where the user passes smaller target buffer
size or requests data from in between. We were disallowing such usage
scenarios before, isn't it?

thanks,
suresh

Re: [PATCH x86/mm] x86: i387 fpregs_set convert_to_fxsr

[Roland McGrath]

> On Thu, Jan 24, 2008 at 05:59:33PM -0800, Roland McGrath wrote:
> > +	if (pos > 0 || count < sizeof(env))
> > +		convert_from_fxsr(&env, target);
> 
> Well, is the generic regset code enforce the need for this now? Can we
> disallow the usage cases where the user passes smaller target buffer
> size or requests data from in between. We were disallowing such usage
> scenarios before, isn't it?

It's true that there was before no way to touch only part of the FPU state
via ptrace.  The user_regset interface genericizes access to every regset
to permit partial access, subject to the size and align fields.  Access to
some part of the data is certainly of use for some regsets, like the
general registers and TLS, which always have exposed means to touch just
one chunk (e.g. PTRACE_POKEUSR, PTRACE_SET_THREAD_AREA).  It's a very
worthwhile thing that we have a uniform interface for all the regsets,
so I certainly would not change that.  

It could fit into the user_regset interface as now defined, to set .n = 1,
.size = whole for the FPU regset, so that all partial access is officially
invalid.  But it is easy enough to support partial access in fact, and for
the common case (xfpregs) there is no overhead or complexity to it at all.
Some new user of the user_regset interfaces very well might have a use for
fetching or touching just one FP register, so why rule it out a priori?


Thanks,
Roland