Re: [Xen-devel] [PATCH] x86: correct EFLAGS.IF in SYSENTER frame

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/03/18 14:13, Jan Beulich wrote:
> Commit 9d1d31ad94 ("x86: slightly reduce Meltdown band-aid overhead")
> moved the STI past the PUSHF. While this isn't an active problem (as we
> force EFLAGS.IF to 1 before exiting to guest context), let's not risk
> internal confusion by finding a PV guest frame with interrupts
> apparently off.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -281,6 +281,8 @@ GLOBAL(sysenter_eflags_saved)
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>  
>          GET_STACK_END(bx)
> +        /* PUSHF above has saved EFLAGS.IF clear (the caller had it set). */
> +        orl   $X86_EFLAGS_IF, UREGS_eflags(%rsp)

For the sake of a single or (which would be beside a line of adjacent
stack accesses anyway), I think it would be better to have this
immediately after sysenter_eflags_saved.  It doesn't have an impact on
speculation safety, and can't plausibly be impacted by SMAP.

It is perhaps not very important, but is it worth encoding this as:

  orb $(X86_EFLAGS_IF >> 8), UREGS_eflags+1(%rsp)

We have a similar pattern when testing the interrupt flag.

Somewhat independently of this patch, I think we should assert that
flags are in the expected state in the return-to-guest path, so we
notice accidental breakage like this more easily.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel