[PATCH] x86: correct EFLAGS.IF in SYSENTER frame

[PATCH] x86: correct EFLAGS.IF in SYSENTER frame

[Jan Beulich]

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@suse.com>

--- 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)
         mov   STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
         neg   %rcx
         jz    .Lsyse_cr3_okay




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

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

[Andrew Cooper]

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@suse.com>
>
> --- 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 16.03.18 at 15:29, <andrew.cooper3@citrix.com> wrote:
> 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@suse.com>
>>
>> --- 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.

Well, I had considered that, but that'll be yet one more separate
place to NOP out later on.

> 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.

Aren't back to back different size writes to the same location
recommended against? Then again, the push is a qword write
already anyway, followed by (currently) a dword write. I can
certainly do that. But let's first agree on the placement.

> 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.

Not sure - nothing was broken here afaict, we just want to play
safe. And as said the exit paths already force EFLAGS.IF to 1.

Jan


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

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

[Andrew Cooper]

On 16/03/18 15:04, Jan Beulich wrote:
>>>> On 16.03.18 at 15:29, <andrew.cooper3@citrix.com> wrote:
>> 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@suse.com>
>>>
>>> --- 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.
> Well, I had considered that, but that'll be yet one more separate
> place to NOP out later on.
>
>> 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.
> Aren't back to back different size writes to the same location
> recommended against? Then again, the push is a qword write
> already anyway, followed by (currently) a dword write. I can
> certainly do that. But let's first agree on the placement.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'm not sufficiently concerned about placement to delay the patch.

>
>> 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.
> Not sure - nothing was broken here afaict, we just want to play
> safe. And as said the exit paths already force EFLAGS.IF to 1.

It is only because of the pessimistic approach in a previous XSA fix
that this isn't an XSA itself.

Despite the safety net, I consider it a bug if such an assert could trip
(and there are several paths we do know of which want fixing).

~Andrew

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

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

[Jan Beulich]

>>> On 16.03.18 at 16:42, <andrew.cooper3@citrix.com> wrote:
> On 16/03/18 15:04, Jan Beulich wrote:
>>>>> On 16.03.18 at 15:29, <andrew.cooper3@citrix.com> wrote:
>>> 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.
>> Not sure - nothing was broken here afaict, we just want to play
>> safe. And as said the exit paths already force EFLAGS.IF to 1.
> 
> It is only because of the pessimistic approach in a previous XSA fix
> that this isn't an XSA itself.

That plus the patch moving the STI didn't go anywhere other than
into staging so far.

Jan


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