* [PATCH 1/2] x86/emul: Replace opencoded extraction of IOPL from eflags
@ 2017-01-06 20:11 Andrew Cooper
2017-01-06 20:11 ` [PATCH 2/2] x86/pv: Check that emulate_privileged_op() didn't change TF Andrew Cooper
2017-01-09 8:43 ` [PATCH 1/2] x86/emul: Replace opencoded extraction of IOPL from eflags Jan Beulich
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Cooper @ 2017-01-06 20:11 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/x86_emulate/x86_emulate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 98ba7c5..c4ba79b 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1157,7 +1157,7 @@ _mode_iopl(
int cpl = get_cpl(ctxt, ops);
if ( cpl == -1 )
return -1;
- return (cpl <= ((ctxt->regs->_eflags >> 12) & 3));
+ return cpl <= MASK_EXTR(ctxt->regs->_eflags, EFLG_IOPL);
}
#define mode_ring0() ({ \
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] x86/pv: Check that emulate_privileged_op() didn't change TF
2017-01-06 20:11 [PATCH 1/2] x86/emul: Replace opencoded extraction of IOPL from eflags Andrew Cooper
@ 2017-01-06 20:11 ` Andrew Cooper
2017-01-09 8:53 ` Jan Beulich
2017-01-09 8:43 ` [PATCH 1/2] x86/emul: Replace opencoded extraction of IOPL from eflags Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2017-01-06 20:11 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
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(-)
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;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] x86/emul: Replace opencoded extraction of IOPL from eflags
2017-01-06 20:11 [PATCH 1/2] x86/emul: Replace opencoded extraction of IOPL from eflags Andrew Cooper
2017-01-06 20:11 ` [PATCH 2/2] x86/pv: Check that emulate_privileged_op() didn't change TF Andrew Cooper
@ 2017-01-09 8:43 ` Jan Beulich
1 sibling, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2017-01-09 8:43 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 06.01.17 at 21:11, <andrew.cooper3@citrix.com> wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] x86/pv: Check that emulate_privileged_op() didn't change TF
2017-01-06 20:11 ` [PATCH 2/2] x86/pv: Check that emulate_privileged_op() didn't change TF Andrew Cooper
@ 2017-01-09 8:53 ` Jan Beulich
2017-01-09 10:48 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-01-09 8:53 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] x86/pv: Check that emulate_privileged_op() didn't change TF
2017-01-09 8:53 ` Jan Beulich
@ 2017-01-09 10:48 ` Andrew Cooper
2017-01-09 11:02 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2017-01-09 10:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] x86/pv: Check that emulate_privileged_op() didn't change TF
2017-01-09 10:48 ` Andrew Cooper
@ 2017-01-09 11:02 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2017-01-09 11:02 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-09 11:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-06 20:11 [PATCH 1/2] x86/emul: Replace opencoded extraction of IOPL from eflags Andrew Cooper
2017-01-06 20:11 ` [PATCH 2/2] x86/pv: Check that emulate_privileged_op() didn't change TF Andrew Cooper
2017-01-09 8:53 ` Jan Beulich
2017-01-09 10:48 ` Andrew Cooper
2017-01-09 11:02 ` Jan Beulich
2017-01-09 8:43 ` [PATCH 1/2] x86/emul: Replace opencoded extraction of IOPL from eflags Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.