* [PATCH] x86: correct EFLAGS.IF in SYSENTER frame
@ 2018-03-16 14:13 Jan Beulich
2018-03-16 14:29 ` Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2018-03-16 14:13 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] x86: correct EFLAGS.IF in SYSENTER frame 2018-03-16 14:13 [PATCH] x86: correct EFLAGS.IF in SYSENTER frame Jan Beulich @ 2018-03-16 14:29 ` Andrew Cooper 2018-03-16 15:04 ` Jan Beulich 0 siblings, 1 reply; 5+ messages in thread From: Andrew Cooper @ 2018-03-16 14:29 UTC (permalink / raw) To: Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: correct EFLAGS.IF in SYSENTER frame 2018-03-16 14:29 ` Andrew Cooper @ 2018-03-16 15:04 ` Jan Beulich 2018-03-16 15:42 ` Andrew Cooper 0 siblings, 1 reply; 5+ messages in thread From: Jan Beulich @ 2018-03-16 15:04 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: correct EFLAGS.IF in SYSENTER frame 2018-03-16 15:04 ` Jan Beulich @ 2018-03-16 15:42 ` Andrew Cooper 2018-03-16 16:26 ` Jan Beulich 0 siblings, 1 reply; 5+ messages in thread From: Andrew Cooper @ 2018-03-16 15:42 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: correct EFLAGS.IF in SYSENTER frame 2018-03-16 15:42 ` Andrew Cooper @ 2018-03-16 16:26 ` Jan Beulich 0 siblings, 0 replies; 5+ messages in thread From: Jan Beulich @ 2018-03-16 16:26 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-16 16:26 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-16 14:13 [PATCH] x86: correct EFLAGS.IF in SYSENTER frame Jan Beulich 2018-03-16 14:29 ` Andrew Cooper 2018-03-16 15:04 ` Jan Beulich 2018-03-16 15:42 ` Andrew Cooper 2018-03-16 16:26 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.