x86/fpu: Delay instruction pointer fixup until after warning

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

Posted by Dave Hansen 3 months, 2 weeks ago


Changes from v1:
 * Fix minor typos
 * Use the more generic and standard ex_handler_default(). Had the
   original code used this helper, the bug would not have been there
   in the first place.

--

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 the
non-FPU-related instruction is #GP'ing.

Do not fix up RIP until after printing the warning. Do this by using
the more generic and standard ex_handler_default().

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Fixes: d5c8028b4788 ("x86/fpu: Reinitialize FPU registers if restoring FPU state fails")
Acked-by: Alison Schofield <alison.schofield@intel.com>
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 |    5 ++---
 1 file changed, 2 insertions(+), 3 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-24 13:58:09.722855233 -0700
+++ b/arch/x86/mm/extable.c	2025-06-24 13:58:09.736856435 -0700
@@ -122,13 +122,12 @@ 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));
 
 	fpu_reset_from_exception_fixup();
-	return true;
+
+	return ex_handler_default(fixup, regs);
 }
 
 /*
_

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

Posted by Chang S. Bae 3 months, 2 weeks ago

On 6/25/2025 6:01 AM, Dave Hansen wrote:
> 
> 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 the
> non-FPU-related instruction is #GP'ing.
> 
> Do not fix up RIP until after printing the warning. Do this by using> the more generic and standard ex_handler_default().

Indeed, the fix looks obvious and correct.

Also, the trick you previously shared for reproducing the fault is very 
useful for testing cases like this.

I would be happy to provide my tag:
Reviewed-by: Chang S. Bae <chang.seok.bae@intel.com>

Thanks,
Chang

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

Posted by Peter Zijlstra 3 months, 2 weeks ago

On Tue, Jun 24, 2025 at 02:01:48PM -0700, Dave Hansen wrote:
> 
> Changes from v1:
>  * Fix minor typos
>  * Use the more generic and standard ex_handler_default(). Had the
>    original code used this helper, the bug would not have been there
>    in the first place.

Doesn't this here typically go under the --- with the diffstat etc?

> --
> 
> 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 the
> non-FPU-related instruction is #GP'ing.
> 
> Do not fix up RIP until after printing the warning. Do this by using
> the more generic and standard ex_handler_default().
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Fixes: d5c8028b4788 ("x86/fpu: Reinitialize FPU registers if restoring FPU state fails")
> Acked-by: Alison Schofield <alison.schofield@intel.com>
> 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>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

Posted by Dave Hansen 3 months, 2 weeks ago

On 6/25/25 01:57, Peter Zijlstra wrote:
> On Tue, Jun 24, 2025 at 02:01:48PM -0700, Dave Hansen wrote:
>> Changes from v1:
>>  * Fix minor typos
>>  * Use the more generic and standard ex_handler_default(). Had the
>>    original code used this helper, the bug would not have been there
>>    in the first place.
> Doesn't this here typically go under the --- with the diffstat etc?

I always put it first because it's the first thing I want the reviewers
to see. I don't like the idea of burying it.

But maybe I'm weird.

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

Posted by Chao Gao 3 months, 2 weeks ago

On Tue, Jun 24, 2025 at 02:01:48PM -0700, Dave Hansen wrote:
>
>Changes from v1:
> * Fix minor typos
> * Use the more generic and standard ex_handler_default(). Had the
>   original code used this helper, the bug would not have been there
>   in the first place.
>
>--
>
>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 the
>non-FPU-related instruction is #GP'ing.
>
>Do not fix up RIP until after printing the warning. Do this by using
>the more generic and standard ex_handler_default().
>
>Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
>Fixes: d5c8028b4788 ("x86/fpu: Reinitialize FPU registers if restoring FPU state fails")
>Acked-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Chao Gao <chao.gao@intel.com>