diff mbox

[2/2] x86/pv: Check that emulate_privileged_op() didn't change TF

Message ID 1483733486-25193-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper Jan. 6, 2017, 8:11 p.m. UTC
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(-)

Comments

Jan Beulich Jan. 9, 2017, 8:53 a.m. UTC | #1
>>> 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
Andrew Cooper Jan. 9, 2017, 10:48 a.m. UTC | #2
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
Jan Beulich Jan. 9, 2017, 11:02 a.m. UTC | #3
>>> 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 mbox

Patch

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;