| Message ID | 1483733486-25193-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.01.17 at 21:11, <andrew.cooper3@citrix.com> wrote: > As the comment says, the guest shouldn't be able to change X86_EFLAGS_TF, > although we don't care which value it currently has. If we don't care about its value, why bother checking? IF and IOPL are different, see XSA-202 (albeit with that workaround, strictly speaking we don't need those checks here anymore - the code was written long before XSA-202 was found). > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -3395,7 +3395,8 @@ static int emulate_privileged_op(struct cpu_user_regs *regs) > * Un-mirror virtualized state from EFLAGS. > * Nothing we allow to be emulated can change TF, IF, or IOPL. > */ > - ASSERT(!((regs->_eflags ^ eflags) & (X86_EFLAGS_IF | X86_EFLAGS_IOPL))); > + ASSERT(!((regs->_eflags ^ eflags) & > + (X86_EFLAGS_TF | X86_EFLAGS_IF | X86_EFLAGS_IOPL))); Actually I did intentionally remove the TF check (together with quite a bit of special handling of TF, as I think was present in early versions of the patch) during the re-base on top of 1d60efc574 ("x86/emul: Implement singlestep as a retire flag"), so I'd rather say the missing comment adjustment is what we want. Or if we were to check flags the values of which we don't care about, then perhaps that should cover all flags which can't legitimately change? Jan
On 09/01/17 08:53, Jan Beulich wrote: >>>> On 06.01.17 at 21:11, <andrew.cooper3@citrix.com> wrote: >> As the comment says, the guest shouldn't be able to change X86_EFLAGS_TF, >> although we don't care which value it currently has. > If we don't care about its value, why bother checking? IF and IOPL > are different, see XSA-202 (albeit with that workaround, strictly > speaking we don't need those checks here anymore - the code was > written long before XSA-202 was found). > >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -3395,7 +3395,8 @@ static int emulate_privileged_op(struct cpu_user_regs *regs) >> * Un-mirror virtualized state from EFLAGS. >> * Nothing we allow to be emulated can change TF, IF, or IOPL. >> */ >> - ASSERT(!((regs->_eflags ^ eflags) & (X86_EFLAGS_IF | X86_EFLAGS_IOPL))); >> + ASSERT(!((regs->_eflags ^ eflags) & >> + (X86_EFLAGS_TF | X86_EFLAGS_IF | X86_EFLAGS_IOPL))); > Actually I did intentionally remove the TF check (together with quite > a bit of special handling of TF, as I think was present in early > versions of the patch) during the re-base on top of 1d60efc574 > ("x86/emul: Implement singlestep as a retire flag"), so I'd rather say > the missing comment adjustment is what we want. > > Or if we were to check flags the values of which we don't care about, > then perhaps that should cover all flags which can't legitimately > change? So something like ASSERT(!((regs->_eflags ^ eflags) & ~X86_EFLAGS_ARITH_MASK)); Might as well check as much as we can, seeing as it is entirely free for us to do so here. ~Andrew
>>> On 09.01.17 at 11:48, <andrew.cooper3@citrix.com> wrote: > On 09/01/17 08:53, Jan Beulich wrote: >>>>> On 06.01.17 at 21:11, <andrew.cooper3@citrix.com> wrote: >>> --- a/xen/arch/x86/traps.c >>> +++ b/xen/arch/x86/traps.c >>> @@ -3395,7 +3395,8 @@ static int emulate_privileged_op(struct cpu_user_regs *regs) >>> * Un-mirror virtualized state from EFLAGS. >>> * Nothing we allow to be emulated can change TF, IF, or IOPL. >>> */ >>> - ASSERT(!((regs->_eflags ^ eflags) & (X86_EFLAGS_IF | X86_EFLAGS_IOPL))); >>> + ASSERT(!((regs->_eflags ^ eflags) & >>> + (X86_EFLAGS_TF | X86_EFLAGS_IF | X86_EFLAGS_IOPL))); >> Actually I did intentionally remove the TF check (together with quite >> a bit of special handling of TF, as I think was present in early >> versions of the patch) during the re-base on top of 1d60efc574 >> ("x86/emul: Implement singlestep as a retire flag"), so I'd rather say >> the missing comment adjustment is what we want. >> >> Or if we were to check flags the values of which we don't care about, >> then perhaps that should cover all flags which can't legitimately >> change? > > So something like > > ASSERT(!((regs->_eflags ^ eflags) & ~X86_EFLAGS_ARITH_MASK)); > > Might as well check as much as we can, seeing as it is entirely free for > us to do so here. That's a good idea I think, let's use this (together with a comment adjustment). Jan
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index e45ff71..6dbdaa0 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -3395,7 +3395,8 @@ static int emulate_privileged_op(struct cpu_user_regs *regs) * Un-mirror virtualized state from EFLAGS. * Nothing we allow to be emulated can change TF, IF, or IOPL. */ - ASSERT(!((regs->_eflags ^ eflags) & (X86_EFLAGS_IF | X86_EFLAGS_IOPL))); + ASSERT(!((regs->_eflags ^ eflags) & + (X86_EFLAGS_TF | X86_EFLAGS_IF | X86_EFLAGS_IOPL))); regs->_eflags |= X86_EFLAGS_IF; regs->_eflags &= ~X86_EFLAGS_IOPL;
As the comment says, the guest shouldn't be able to change X86_EFLAGS_TF, although we don't care which value it currently has. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/traps.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)