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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/01/17 08:53, Jan Beulich wrote:
>>>> 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?

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

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