[RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE on sigentry

[RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE on sigentry

[Sebastian Andrzej Siewior]

This commit reverts commit 04944b793e18e ("x86: xsave: set FP, SSE bits
in the xsave header in the user sigcontext"). The commit claims that it
is required for legacy applications but fails to explain why this is
needed and it is not obvious to me why the application would require the
FP/SSE state in the signal handler.

In the compacted form, the XSAVES may save only XMM+SSE but skip FP.
This is denoted by header->xfeatures = 6. The fastpath
(copy_fpregs_to_sigframe()) does that but _also_ initialises the FP
state (cwd to 0x37f, mxcsr as we do, remaining fields to 0).
The slowpath (copy_xstate_to_user()) leaves most of the FP untouched.
Only mxcsr and mxcsr_flags are set due to xfeatures_mxcsr_quirk().
Now that XFEATURE_MASK_FP is set unconditionally we load on return from
signal random garbage as the FP state.
This results in SIGFPE on one particular machine (the same testcase
(starting a java application) on older generations seems to work).

One way to fix this is not to unconditionally set XFEATURE_MASK_FPSSE
since it was never saved. This avoids loading garbage on the return
path.
We could also initialise the FP state in the xfeatures_mxcsr_quirk()
case like xsave does.

Reported-by: Kurt Kanzenbach <kurt.kanzenbach@linutronix.de>
Fixes: 69277c98f5eef ("x86/fpu: Always store the registers in copy_fpstate_to_sigframe()")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 7026f1c4e5e30..6c66203b19f3c 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -81,7 +81,6 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
 {
 	struct xregs_state __user *x = buf;
 	struct _fpx_sw_bytes *sw_bytes;
-	u32 xfeatures;
 	int err;
 
 	/* Setup the bytes not touched by the [f]xsave and reserved for SW. */
@@ -93,28 +92,6 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
 
 	err |= __put_user(FP_XSTATE_MAGIC2,
 			  (__u32 __user *)(buf + fpu_user_xstate_size));
-
-	/*
-	 * Read the xfeatures which we copied (directly from the cpu or
-	 * from the state in task struct) to the user buffers.
-	 */
-	err |= __get_user(xfeatures, (__u32 __user *)&x->header.xfeatures);
-
-	/*
-	 * For legacy compatible, we always set FP/SSE bits in the bit
-	 * vector while saving the state to the user context. This will
-	 * enable us capturing any changes(during sigreturn) to
-	 * the FP/SSE bits by the legacy applications which don't touch
-	 * xfeatures in the xsave header.
-	 *
-	 * xsave aware apps can change the xfeatures in the xsave
-	 * header as well as change any contents in the memory layout.
-	 * xrestore as part of sigreturn will capture all the changes.
-	 */
-	xfeatures |= XFEATURE_MASK_FPSSE;
-
-	err |= __put_user(xfeatures, (__u32 __user *)&x->header.xfeatures);
-
 	return err;
 }
 
-- 
2.20.1

Re: [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE on sigentry

[Dave Hansen]

On 4/25/19 10:35 AM, Sebastian Andrzej Siewior wrote:
> This commit reverts commit 04944b793e18e ("x86: xsave: set FP, SSE bits
> in the xsave header in the user sigcontext"). The commit claims that it
> is required for legacy applications but fails to explain why this is
> needed and it is not obvious to me why the application would require the
> FP/SSE state in the signal handler.

Any software that understands XSAVE is OK.  I think the legacy software
would be that which groks 'fxregs_state, and FXSAVE/FXRSTOR but does not
comprehend XSAVE/XRSTOR.  *That* software might change fxregs_state in
the signal frame, but the lack of XFEATURE_MASK_FPSSE in xfeatures would
prevent XRSTOR from restoring it.

That's just a guess, though.

If we care, I think we should just use XSAVE instead of XSAVEOPT and
trying to reconstruct the init state in software.

Re: [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE on sigentry

[Sebastian Andrzej Siewior]

On 2019-04-25 14:13:05 [-0700], Dave Hansen wrote:
> On 4/25/19 10:35 AM, Sebastian Andrzej Siewior wrote:
> > This commit reverts commit 04944b793e18e ("x86: xsave: set FP, SSE bits
> > in the xsave header in the user sigcontext"). The commit claims that it
> > is required for legacy applications but fails to explain why this is
> > needed and it is not obvious to me why the application would require the
> > FP/SSE state in the signal handler.
> 
> Any software that understands XSAVE is OK.  I think the legacy software
> would be that which groks 'fxregs_state, and FXSAVE/FXRSTOR but does not
> comprehend XSAVE/XRSTOR.  *That* software might change fxregs_state in
> the signal frame, but the lack of XFEATURE_MASK_FPSSE in xfeatures would
> prevent XRSTOR from restoring it.

but it would edit its FP state before it has been used.

> That's just a guess, though.
> 
> If we care, I think we should just use XSAVE instead of XSAVEOPT and
> trying to reconstruct the init state in software.

We can't use XSAVE directly in the slowpath. We need to reconstruct the
init state. We have the mxcsr quirk. We would need just to extend it and
set the FP area to init state if the FP state is missing like we do in
fpstate_sanitize_xstate().

Sebastian

Re: [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE on sigentry

[Dave Hansen]

On 4/26/19 12:26 AM, Sebastian Andrzej Siewior wrote:
>> That's just a guess, though.
>>
>> If we care, I think we should just use XSAVE instead of XSAVEOPT and
>> trying to reconstruct the init state in software.
> We can't use XSAVE directly in the slowpath. We need to reconstruct the
> init state. We have the mxcsr quirk. We would need just to extend it and
> set the FP area to init state if the FP state is missing like we do in
> fpstate_sanitize_xstate().

Can you remind me why we can't use XSAVE directly in the slow path?

Re: [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE on sigentry

[Sebastian Andrzej Siewior]

On 2019-04-26 09:33:28 [-0700], Dave Hansen wrote:
> On 4/26/19 12:26 AM, Sebastian Andrzej Siewior wrote:
> >> That's just a guess, though.
> >>
> >> If we care, I think we should just use XSAVE instead of XSAVEOPT and
> >> trying to reconstruct the init state in software.
> > We can't use XSAVE directly in the slowpath. We need to reconstruct the
> > init state. We have the mxcsr quirk. We would need just to extend it and
> > set the FP area to init state if the FP state is missing like we do in
> > fpstate_sanitize_xstate().
> 
> Can you remind me why we can't use XSAVE directly in the slow path?

Where to?
In the fastpath we XSAVE directly to task's stack. We are in the
slowpath because this failed. Task's FPU-state is using compacted form.
So we use this as source and copy_to_user() to task's stack.
I don't think we can XSAVE to task's FPU-state because the compacted
form may need less memory than the non-compacted form.

Currently I'm leaning towards cleaning the FP area so we behave like
XSAVE does. Independently of that, I would like to revert that commit.
Based on the comment and patch description it does not say that it fixes
a real problem. It *may* fix something.

Sebastian

Re: [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE on sigentry

[Dave Hansen]

On 4/26/19 9:50 AM, Sebastian Andrzej Siewior wrote:
> On 2019-04-26 09:33:28 [-0700], Dave Hansen wrote:
>> On 4/26/19 12:26 AM, Sebastian Andrzej Siewior wrote:
>>>> That's just a guess, though.
>>>>
>>>> If we care, I think we should just use XSAVE instead of XSAVEOPT and
>>>> trying to reconstruct the init state in software.
>>> We can't use XSAVE directly in the slowpath. We need to reconstruct the
>>> init state. We have the mxcsr quirk. We would need just to extend it and
>>> set the FP area to init state if the FP state is missing like we do in
>>> fpstate_sanitize_xstate().
>>
>> Can you remind me why we can't use XSAVE directly in the slow path?
> 
> Where to? In the fastpath we XSAVE directly to task's stack. We are
> in the slowpath because this failed.
I'm looking at the code and having a bit of a hard time connecting it to
what you're saying here.

We are in copy_fpstate_to_sigframe(), right?  Let's assume we are
using_compacted_format().  If copy_fpregs_to_sigframe() fails to copy,
we return immediately and never get to the save_xstate_epilog() code in
question.

So, to even get to save_xstate_epilog(), we had to do a *successful*
copy_fpstate_to_sigframe() which, on XSAVE systems will use
copy_xregs_to_user() which already uses plain XSAVE (not XSAVEOPT).

save_xstate_epilog() goes and tries to set XFEATURE_MASK_FPSSE on this
XSAVE (literally the XSAVE instruction) generated header.

But, if we're dealing with header.xfeatures generated by an XSAVE with
the RFBM=-1, I don't understand how header.xfeatures could ever *not*
have XFEATURE_MASK_FPSSE set.  The only way would be if we had gotten
here after using FXSAVE, but I believe that's impossible since those
systems have use_xsave()==0.

IOW, I think the patch is right, but I'm not sure I totally agree with
the reasoning.

Re: [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE on sigentry

[Sebastian Andrzej Siewior]

On 2019-04-26 12:04:55 [-0700], Dave Hansen wrote:
> I'm looking at the code and having a bit of a hard time connecting it to
> what you're saying here.
> 
> We are in copy_fpstate_to_sigframe(), right?  Let's assume we are
> using_compacted_format().  If copy_fpregs_to_sigframe() fails to copy,
> we return immediately and never get to the save_xstate_epilog() code in
> question.
> 
> So, to even get to save_xstate_epilog(), we had to do a *successful*
> copy_fpstate_to_sigframe() which, on XSAVE systems will use
> copy_xregs_to_user() which already uses plain XSAVE (not XSAVEOPT).

We are in:

copy_fpstate_to_sigframe()
 |
 copy_fpregs_to_sigframe() fails.
 |
 using_compacted_format()
  |
  copy_xstate_to_user()
   |
   __copy_xstate_to_user() the xstate_header
   |
   for (i = 0; i < XFEATURE_MAX; i++)
    copy only XFEATURE_SSE + XFEATURE_YMM, other aren't set.
   |
   xfeatures_mxcsr_quirk() true, we copy
   |
   __copy_xstate_to_user() the xstate_fx_sw_bytes

Now. The FP part of the xsave has never been touched which means the
bytes are not initialized. The are is filled with random conten on user
stack. The saved xfeatures say only (XFEATURE_SSE | XFEATURE_YMM) so a
XRESTOR of that gives us exactly what we stored.

> save_xstate_epilog() goes and tries to set XFEATURE_MASK_FPSSE on this
> XSAVE (literally the XSAVE instruction) generated header.

Correct. But we never saved the "data" for XFEATURE_MASK_FP and, as noted
above, the content is random garbage on the stack.
Be setting XFEATURE_MASK_FPSSE we claim that suddenly that the FP part
of the XSAVE area is valid which is not the case.

> But, if we're dealing with header.xfeatures generated by an XSAVE with
> the RFBM=-1, I don't understand how header.xfeatures could ever *not*
> have XFEATURE_MASK_FPSSE set.  The only way would be if we had gotten
> here after using FXSAVE, but I believe that's impossible since those
> systems have use_xsave()==0.

Look at the code path above. XSAVES stored the state in task's
&fpu->state.xsave using XSAVES and we copied the *saved* bits so it
looks like XSAVE. XFEATURE_MASK_FP was not set, so we skipped that area.

> IOW, I think the patch is right, but I'm not sure I totally agree with
> the reasoning.

As I described in the path description: XSAVE (the fastpath, the code
before I started touching it) would also not set XFEATURE_MASK_FP in
xfeatures *but* would set FP area to its ini-state. Therefore the
unconditional OR of XFEATURE_MASK_FPSSE does not hurt because the FP
area was initialized to its initstate. The following restore would load
a valid FP state.

Sebastian

Re: [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK_FPSSE on sigentry

[Dave Hansen]

On 4/26/19 1:05 PM, Sebastian Andrzej Siewior wrote:
> 
> copy_fpstate_to_sigframe()
>  |
>  copy_fpregs_to_sigframe() fails.
>  |
>  using_compacted_format()

Aw, crud.  I was looking at Linus's tree.  Sorry about that.  In Linus's
tree, if copy_fpregs_to_sigframe() fails, we just return from
copy_fpstate_to_sigframe() immediately.  In other words, either XSAVE
works, and we don't have this issue, or XSAVE fails and we don't do the
fixup code in question.

I'll say it again, though: I think we should be using the XSAVE
instruction to save this state.  If we need to map some pages in order
to make XSAVE work, then I think we should bring some pages in and we
can do *that* in the slow path.  We can even do that without taking page
faults by just calling get_user_pages() for instance.

Or, the slow path could be to fpregs_unlock(), zero the user area
(taking and handling page faults), then fpregs_lock() and retry the
copy_fpregs_to_sigframe().

[PATCH] x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails

[Sebastian Andrzej Siewior]

In the compacted form, the XSAVES may save only XMM+SSE but skip FP.
This is denoted by header->xfeatures = 6. The fastpath
(copy_fpregs_to_sigframe()) does that but _also_ initialises the FP
state (cwd to 0x37f, mxcsr as we do, remaining fields to 0).
The slowpath (copy_xstate_to_user()) leaves most of the FP untouched.
Only mxcsr and mxcsr_flags are set due to xfeatures_mxcsr_quirk().
Now that XFEATURE_MASK_FP is set unconditionally we load on return from
signal random garbage as the FP state.

Instead of utilizing copy_xstate_to_user() fault-in the user memory and
retry the fast path. Ideally the fast path succeeds on the second
attempt but may be retried again if the memory is swapped out due to
memory pressure. If the user memory can not be faulted-in then
get_user_pages() returns an error so we don't loop forever.

Fault in memory via get_user_pages() so copy_fpregs_to_sigframe()
succeeds without a fault.

Reported-by: Kurt Kanzenbach <kurt.kanzenbach@linutronix.de>
Suggested-by: Dave Hansen <dave.hansen@intel.com>
Fixes: 69277c98f5eef ("x86/fpu: Always store the registers in copy_fpstate_to_sigframe()")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 7026f1c4e5e30..6d6c2d6afde42 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -158,7 +158,6 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
 int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 {
 	struct fpu *fpu = &current->thread.fpu;
-	struct xregs_state *xsave = &fpu->state.xsave;
 	struct task_struct *tsk = current;
 	int ia32_fxstate = (buf != buf_fx);
 	int ret = -EFAULT;
@@ -174,11 +173,12 @@ 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;
 
+retry:
 	/*
 	 * Load the FPU registers if they are not valid for the current task.
 	 * With a valid FPU state we can attempt to save the state directly to
-	 * userland's stack frame which will likely succeed. If it does not, do
-	 * the slowpath.
+	 * userland's stack frame which will likely succeed. If it does not,
+	 * resolve the fault in the user memory and try again.
 	 */
 	fpregs_lock();
 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
@@ -193,14 +193,17 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 	fpregs_unlock();
 
 	if (ret) {
-		if (using_compacted_format()) {
-			if (copy_xstate_to_user(buf_fx, xsave, 0, size))
-				return -1;
-		} else {
-			fpstate_sanitize_xstate(fpu);
-			if (__copy_to_user(buf_fx, xsave, fpu_user_xstate_size))
-				return -1;
-		}
+		int aligned_size;
+		int nr_pages;
+
+		aligned_size = offset_in_page(buf_fx) + fpu_user_xstate_size;
+		nr_pages = DIV_ROUND_UP(aligned_size, PAGE_SIZE);
+
+		ret = get_user_pages((unsigned long)buf_fx, nr_pages,
+				     FOLL_WRITE, NULL, NULL);
+		if (ret == nr_pages)
+			goto retry;
+		return -EFAULT;
 	}
 
 	/* Save the fsave header for the 32-bit frames. */
-- 
2.20.1

[tip:x86/fpu] x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails

[tip-bot for Sebastian Andrzej Siewior]

Commit-ID:  eeec00d73be2e92ebce16c89154726250f2c80ef
Gitweb:     https://git.kernel.org/tip/eeec00d73be2e92ebce16c89154726250f2c80ef
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Mon, 29 Apr 2019 18:39:53 +0200
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Mon, 29 Apr 2019 20:25:45 +0200

x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails

In the compacted form, XSAVES may save only the XMM+SSE state but skip
FP (x87 state).

This is denoted by header->xfeatures = 6. The fastpath
(copy_fpregs_to_sigframe()) does that but _also_ initialises the FP
state (cwd to 0x37f, mxcsr as we do, remaining fields to 0).

The slowpath (copy_xstate_to_user()) leaves most of the FP
state untouched. Only mxcsr and mxcsr_flags are set due to
xfeatures_mxcsr_quirk(). Now that XFEATURE_MASK_FP is set
unconditionally, see

  04944b793e18 ("x86: xsave: set FP, SSE bits in the xsave header in the user sigcontext"),

on return from the signal, random garbage is loaded as the FP state.

Instead of utilizing copy_xstate_to_user(), fault-in the user memory
and retry the fast path. Ideally, the fast path succeeds on the second
attempt but may be retried again if the memory is swapped out due
to memory pressure. If the user memory can not be faulted-in then
get_user_pages() returns an error so we don't loop forever.

Fault in memory via get_user_pages() so copy_fpregs_to_sigframe()
succeeds without a fault.

Fixes: 69277c98f5eef ("x86/fpu: Always store the registers in copy_fpstate_to_sigframe()")
Reported-by: Kurt Kanzenbach <kurt.kanzenbach@linutronix.de>
Suggested-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jann Horn <jannh@google.com>
Cc: Jason@zx2c4.com
Cc: kvm ML <kvm@vger.kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Rik van Riel <riel@surriel.com>
Cc: rkrcmar@redhat.com
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20190429163953.gqxgsc5okqxp4olv@linutronix.de
---
 arch/x86/kernel/fpu/signal.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 7026f1c4e5e3..6d6c2d6afde4 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -158,7 +158,6 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
 int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 {
 	struct fpu *fpu = &current->thread.fpu;
-	struct xregs_state *xsave = &fpu->state.xsave;
 	struct task_struct *tsk = current;
 	int ia32_fxstate = (buf != buf_fx);
 	int ret = -EFAULT;
@@ -174,11 +173,12 @@ 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;
 
+retry:
 	/*
 	 * Load the FPU registers if they are not valid for the current task.
 	 * With a valid FPU state we can attempt to save the state directly to
-	 * userland's stack frame which will likely succeed. If it does not, do
-	 * the slowpath.
+	 * userland's stack frame which will likely succeed. If it does not,
+	 * resolve the fault in the user memory and try again.
 	 */
 	fpregs_lock();
 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
@@ -193,14 +193,17 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 	fpregs_unlock();
 
 	if (ret) {
-		if (using_compacted_format()) {
-			if (copy_xstate_to_user(buf_fx, xsave, 0, size))
-				return -1;
-		} else {
-			fpstate_sanitize_xstate(fpu);
-			if (__copy_to_user(buf_fx, xsave, fpu_user_xstate_size))
-				return -1;
-		}
+		int aligned_size;
+		int nr_pages;
+
+		aligned_size = offset_in_page(buf_fx) + fpu_user_xstate_size;
+		nr_pages = DIV_ROUND_UP(aligned_size, PAGE_SIZE);
+
+		ret = get_user_pages((unsigned long)buf_fx, nr_pages,
+				     FOLL_WRITE, NULL, NULL);
+		if (ret == nr_pages)
+			goto retry;
+		return -EFAULT;
 	}
 
 	/* Save the fsave header for the 32-bit frames. */

Re: [tip:x86/fpu] x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails

[Dave Hansen]

On 4/29/19 11:52 AM, tip-bot for Sebastian Andrzej Siewior wrote:
> Instead of utilizing copy_xstate_to_user(), fault-in the user memory
> and retry the fast path. Ideally, the fast path succeeds on the second
> attempt but may be retried again if the memory is swapped out due
> to memory pressure. If the user memory can not be faulted-in then
> get_user_pages() returns an error so we don't loop forever.
> 
> Fault in memory via get_user_pages() so copy_fpregs_to_sigframe()
> succeeds without a fault.

Thanks for reworking this.  It looks great.

Acked-by: Dave Hansen <dave.hansen@intel.com>