Re: [Xen-devel] [PATCH 2/2] x86/pv: Check that emulate_privileged_op() didn't change TF

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.01.17 at 21:11, <andrew.cooper3@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel