[PATCH] x86/fpu: Delay instruction pointer fixup until after after warning

[PATCH] x86/fpu: Delay instruction pointer fixup until after after warning

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Right now, if XRSTOR fails a console message like this is be printed:

	Bad FPU state detected at restore_fpregs_from_fpstate+0x9a/0x170, reinitializing FPU registers.

However, the text location (...+0x9a in this case) is the instruction
*AFTER* the XRSTOR. The highlighted instruction in the "Code:" dump
also points one instruction late.

The reason is that the "fixup" moves RIP up to pass the bad XRSTOR
and keep on running after returning from the #GP handler. But it
does this fixup before warning.

The resulting warning output is nonsensical because it looks like
e non-FPU-related instruction is #GP'ing.

Do not fix up RIP until after printing the warning.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Fixes: d5c8028b4788 ("x86/fpu: Reinitialize FPU registers if restoring FPU state fails")
Cc: stable@vger.kernel.org
Cc: Eric Biggers <ebiggers@google.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Chang S. Bae <chang.seok.bae@intel.com>
---

 b/arch/x86/mm/extable.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN arch/x86/mm/extable.c~fixup-fpu-gp-ip-later arch/x86/mm/extable.c
--- a/arch/x86/mm/extable.c~fixup-fpu-gp-ip-later	2025-06-18 12:21:30.231719499 -0700
+++ b/arch/x86/mm/extable.c	2025-06-18 12:25:53.979954060 -0700
@@ -122,11 +122,11 @@ static bool ex_handler_sgx(const struct
 static bool ex_handler_fprestore(const struct exception_table_entry *fixup,
 				 struct pt_regs *regs)
 {
-	regs->ip = ex_fixup_addr(fixup);
-
 	WARN_ONCE(1, "Bad FPU state detected at %pB, reinitializing FPU registers.",
 		  (void *)instruction_pointer(regs));
 
+	regs->ip = ex_fixup_addr(fixup);
+
 	fpu_reset_from_exception_fixup();
 	return true;
 }
_

Re: [PATCH] x86/fpu: Delay instruction pointer fixup until after after warning

[Alison Schofield]

On Wed, Jun 18, 2025 at 12:33:13PM -0700, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>

In the subject line: s/after after/after

> 
> Right now, if XRSTOR fails a console message like this is be printed:
> 
> 	Bad FPU state detected at restore_fpregs_from_fpstate+0x9a/0x170, reinitializing FPU registers.
> 
> However, the text location (...+0x9a in this case) is the instruction
> *AFTER* the XRSTOR. The highlighted instruction in the "Code:" dump
> also points one instruction late.
> 
> The reason is that the "fixup" moves RIP up to pass the bad XRSTOR
> and keep on running after returning from the #GP handler. But it
> does this fixup before warning.
> 
> The resulting warning output is nonsensical because it looks like
> e non-FPU-related instruction is #GP'ing.

s/e/the

> 
> Do not fix up RIP until after printing the warning.

How was this found and how is the change verified?
ie. do we have a mechanism for hitting this path easily?

With the grammar cleanups,
Acked-by: Alison Schofield <alison.schofield@intel.com>


> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Fixes: d5c8028b4788 ("x86/fpu: Reinitialize FPU registers if restoring FPU state fails")
> Cc: stable@vger.kernel.org
> Cc: Eric Biggers <ebiggers@google.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Chang S. Bae <chang.seok.bae@intel.com>
> ---
> 
>  b/arch/x86/mm/extable.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff -puN arch/x86/mm/extable.c~fixup-fpu-gp-ip-later arch/x86/mm/extable.c
> --- a/arch/x86/mm/extable.c~fixup-fpu-gp-ip-later	2025-06-18 12:21:30.231719499 -0700
> +++ b/arch/x86/mm/extable.c	2025-06-18 12:25:53.979954060 -0700
> @@ -122,11 +122,11 @@ static bool ex_handler_sgx(const struct
>  static bool ex_handler_fprestore(const struct exception_table_entry *fixup,
>  				 struct pt_regs *regs)
>  {
> -	regs->ip = ex_fixup_addr(fixup);
> -
>  	WARN_ONCE(1, "Bad FPU state detected at %pB, reinitializing FPU registers.",
>  		  (void *)instruction_pointer(regs));
>  
> +	regs->ip = ex_fixup_addr(fixup);
> +
>  	fpu_reset_from_exception_fixup();
>  	return true;
>  }
> _

Re: [PATCH] x86/fpu: Delay instruction pointer fixup until after after warning

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

On 6/18/25 12:51, Alison Schofield wrote:
>> Do not fix up RIP until after printing the warning.
> How was this found and how is the change verified?

Good questions.

I found it from an Intel-internal bug report. It's not clear what's
causing the underlying XRSTOR #GP. But I spent some time scratching my
head about how RIP got pointing to the wrong place. I was blaming the
simulator at first.

I validated the fix using the attached patch. It waits until there's a
program named "dave" running, then corrupts the XSAVE buffer in a way
that will cause XRSTOR to #GP, triggering the warning that was off by an
instruction.

[-- Attachment #2: os_rstor_fun.patch --]
[-- Type: text/x-patch, Size: 576 bytes --]



---

 b/arch/x86/kernel/fpu/core.c |    3 +++
 1 file changed, 3 insertions(+)

diff -puN arch/x86/kernel/fpu/core.c~os_rstor_fun arch/x86/kernel/fpu/core.c
--- a/arch/x86/kernel/fpu/core.c~os_rstor_fun	2025-06-18 11:22:58.583510842 -0700
+++ b/arch/x86/kernel/fpu/core.c	2025-06-18 11:23:46.626730032 -0700
@@ -202,6 +202,9 @@ void restore_fpregs_from_fpstate(struct
 		 */
 		mask = fpu_kernel_cfg.max_features & mask;
 
+		if (!strncmp(current->comm, "dave", 4))
+			fpstate->regs.xsave.header.xcomp_bv = 0;
+
 		os_xrstor(fpstate, mask);
 	} else {
 		if (use_fxsr())
_

Re: [PATCH] x86/fpu: Delay instruction pointer fixup until after after warning

[Chao Gao]

nit: s/after after/after/ in the subject line

On Wed, Jun 18, 2025 at 12:33:13PM -0700, Dave Hansen wrote:
>
>From: Dave Hansen <dave.hansen@linux.intel.com>
>
>Right now, if XRSTOR fails a console message like this is be printed:
>
>	Bad FPU state detected at restore_fpregs_from_fpstate+0x9a/0x170, reinitializing FPU registers.
>
>However, the text location (...+0x9a in this case) is the instruction
>*AFTER* the XRSTOR. The highlighted instruction in the "Code:" dump
>also points one instruction late.
>
>The reason is that the "fixup" moves RIP up to pass the bad XRSTOR
>and keep on running after returning from the #GP handler. But it
>does this fixup before warning.
>
>The resulting warning output is nonsensical because it looks like
>e non-FPU-related instruction is #GP'ing.
>
>Do not fix up RIP until after printing the warning.
>
>Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
>Fixes: d5c8028b4788 ("x86/fpu: Reinitialize FPU registers if restoring FPU state fails")
>Cc: stable@vger.kernel.org
>Cc: Eric Biggers <ebiggers@google.com>
>Cc: Rik van Riel <riel@redhat.com>
>Cc: Borislav Petkov <bp@alien8.de>
>Cc: Chang S. Bae <chang.seok.bae@intel.com>
>---
>
> b/arch/x86/mm/extable.c |    4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff -puN arch/x86/mm/extable.c~fixup-fpu-gp-ip-later arch/x86/mm/extable.c
>--- a/arch/x86/mm/extable.c~fixup-fpu-gp-ip-later	2025-06-18 12:21:30.231719499 -0700
>+++ b/arch/x86/mm/extable.c	2025-06-18 12:25:53.979954060 -0700
>@@ -122,11 +122,11 @@ static bool ex_handler_sgx(const struct
> static bool ex_handler_fprestore(const struct exception_table_entry *fixup,
> 				 struct pt_regs *regs)
> {
>-	regs->ip = ex_fixup_addr(fixup);
>-
> 	WARN_ONCE(1, "Bad FPU state detected at %pB, reinitializing FPU registers.",
> 		  (void *)instruction_pointer(regs));
> 
>+	regs->ip = ex_fixup_addr(fixup);
>+

instead of delaying the RIP fixup,

> 	fpu_reset_from_exception_fixup();
> 	return true;

can we do

	return ex_handler_default(fixup, regs);

here? Similar to what other handlers ex_handler_{fault, sgx, uaccess, ...} are
doing.

> }
>_

Re: [PATCH] x86/fpu: Delay instruction pointer fixup until after after warning

[Dave Hansen]

On 6/18/25 19:37, Chao Gao wrote:
> instead of delaying the RIP fixup,
> 
>> 	fpu_reset_from_exception_fixup();
>> 	return true;
> can we do
> 
> 	return ex_handler_default(fixup, regs);
> 
> here? Similar to what other handlers ex_handler_{fault, sgx, uaccess, ...} are
> doing.

Yep, good idea. I don't see any reason that this should have been
special in the first place.