Re: [PATCH 2/2] x86/eventinj: Push SP to IRET frame

[Mathias Krause <minipli@grsecurity.net>]

Am 15.09.25 um 16:49 schrieb Chao Gao:
> Push the current stack pointer (SP) to the IRET frame in do_iret() to
> ensure a valid stack after IRET execution, particularly in 64-bit mode.
> 
> Currently, do_iret() crafts an IRET frame with a zeroed SP. For 32-bit
> guests, SP is not popped during IRET due to no privilege change. so, the
> stack after IRET is still valid. But for 64-bit guests, IRET
> unconditionally pops RSP, restoring it to zero. This can cause a nested NMI
> to push its interrupt frame to the topmost page (0xffffffff_fffff000),
> which may be not mapped and cause triple fault [1].

Nice catch!

> 
> To fix this issue, push the current SP to the IRET frame, ensuring RSP is
> restored to a valid stack in 64-bit mode.
> 
> Reported-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Link: https://lore.kernel.org/kvm/aMahfvF1r39Xq6zK@intel.com/
> ---
>  x86/eventinj.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/x86/eventinj.c b/x86/eventinj.c
> index ec8a5ef1..63ebbaab 100644
> --- a/x86/eventinj.c
> +++ b/x86/eventinj.c
> @@ -153,6 +153,7 @@ asm("do_iret:"
>  	"mov 8(%esp), %edx \n\t"	// virt_stack
>  #endif
>  	"xchg %"R "dx, %"R "sp \n\t"	// point to new stack
> +	"push"W" %"R "sp \n\t"

We should also push SS, for consistency reasons. Not that it would
matter much for x64-64, but a NULL selector for SS still feels wrong.

>  	"pushf"W" \n\t"
>  	"mov %cs, %ecx \n\t"
>  	"push"W" %"R "cx \n\t"

The leading comment also needs an update and the 'extern bool
no_test_device' can be dropped as well, as fwcfg.h gets included by
eventinj.c.

So, maybe something like this on top?:

diff --git a/x86/eventinj.c b/x86/eventinj.c
index 63ebbaabda66..4e21f3915820 100644
--- a/x86/eventinj.c
+++ b/x86/eventinj.c
@@ -139,11 +139,8 @@ static void nested_nmi_iret_isr(struct ex_regs *r)

 extern void do_iret(ulong phys_stack, void *virt_stack);

-// Return to same privilege level won't pop SS or SP, so
+// Return to same privilege level won't pop SS or SP for i386 but x86-64, so
 // save it in RDX while we run on the nested stack
-
-extern bool no_test_device;
-
 asm("do_iret:"
 #ifdef __x86_64__
        "mov %rdi, %rax \n\t"           // phys_stack
@@ -153,7 +150,12 @@ asm("do_iret:"
        "mov 8(%esp), %edx \n\t"        // virt_stack
 #endif
        "xchg %"R "dx, %"R "sp \n\t"    // point to new stack
+#ifdef __x86_64__
+       // IRET in 64 bit mode unconditionally pops SS:xSP
+       "mov %ss, %ecx \n\t"
+       "push"W" %"R "cx \n\t"
        "push"W" %"R "sp \n\t"
+#endif
        "pushf"W" \n\t"
        "mov %cs, %ecx \n\t"
        "push"W" %"R "cx \n\t"

Thanks,
Mathias