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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.01.17 at 11:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 09/01/17 08:53, Jan Beulich wrote:
>>>>> On 06.01.17 at 21:11, <andrew.cooper3@xxxxxxxxxx> 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


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