[PATCH] x86/emulate: Don't assume that addr_size == 32 implies protected mode

[PATCH] x86/emulate: Don't assume that addr_size == 32 implies protected mode

[George Dunlap]

Callers of x86_emulate() generally define addr_size based on the code
segment.  In vm86 mode, the code segment is set by the hardware to be
16-bits; but it is entirely possible to enable protected mode, set the
CS to 32-bits, and then disable protected mode.  (This is commonly
called "unreal mode".)

But the instruction decoder only checks for protected mode when
addr_size == 16.  So in unreal mode, hardware will throw a #UD for VEX
prefixes, but our instruction decoder will decode them, triggering an
ASSERT() further on in _get_fpu().  (With debug=n the emulator will
incorrectly emulate the instruction rather than throwing a #UD, but
this is only a bug, not a crash, so it's not a security issue.)

Teach the instruction decoder to check that we're in protected mode,
even if addr_size is 32.

While we're here, replace the open-coded protected mode check with
in_protmode().

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index dfdcd6c..46232c4 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2149,11 +2149,8 @@ x86_decode(
             default:
                 BUG(); /* Shouldn't be possible. */
             case 2:
-                if ( in_realmode(ctxt, ops) || (state->regs->eflags & EFLG_VM) )
-                    break;
-                /* fall through */
             case 4:
-                if ( modrm_mod != 3 )
+                if ( modrm_mod != 3 || !in_protmode(ctxt, ops) )
                     break;
                 /* fall through */
             case 8:
-- 
2.10.0


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

Re: [PATCH] x86/emulate: Don't assume that addr_size == 32 implies protected mode

[Jan Beulich]

>>> On 16.12.16 at 10:55, <george.dunlap@citrix.com> wrote:
> Callers of x86_emulate() generally define addr_size based on the code
> segment.  In vm86 mode, the code segment is set by the hardware to be
> 16-bits; but it is entirely possible to enable protected mode, set the
> CS to 32-bits, and then disable protected mode.  (This is commonly
> called "unreal mode".)

To better match this description I think it would be preferable ...

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -2149,11 +2149,8 @@ x86_decode(
>              default:
>                  BUG(); /* Shouldn't be possible. */
>              case 2:
> -                if ( in_realmode(ctxt, ops) || (state->regs->eflags & EFLG_VM) )
> -                    break;
> -                /* fall through */
>              case 4:
> -                if ( modrm_mod != 3 )
> +                if ( modrm_mod != 3 || !in_protmode(ctxt, ops) )
>                      break;

... to keep the EFLAGS.VM in case 2, and check in_realmode()
in case 4. Otoh what you have now is the more compact form,
resulting in fewer branches ...

Jan


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

Re: [PATCH] x86/emulate: Don't assume that addr_size == 32 implies protected mode

[George Dunlap]

On 16/12/16 10:34, Jan Beulich wrote:
>>>> On 16.12.16 at 10:55, <george.dunlap@citrix.com> wrote:
>> Callers of x86_emulate() generally define addr_size based on the code
>> segment.  In vm86 mode, the code segment is set by the hardware to be
>> 16-bits; but it is entirely possible to enable protected mode, set the
>> CS to 32-bits, and then disable protected mode.  (This is commonly
>> called "unreal mode".)
> 
> To better match this description I think it would be preferable ...
> 
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -2149,11 +2149,8 @@ x86_decode(
>>              default:
>>                  BUG(); /* Shouldn't be possible. */
>>              case 2:
>> -                if ( in_realmode(ctxt, ops) || (state->regs->eflags & EFLG_VM) )
>> -                    break;
>> -                /* fall through */
>>              case 4:
>> -                if ( modrm_mod != 3 )
>> +                if ( modrm_mod != 3 || !in_protmode(ctxt, ops) )
>>                      break;
> 
> ... to keep the EFLAGS.VM in case 2, and check in_realmode()
> in case 4. Otoh what you have now is the more compact form,
> resulting in fewer branches ...

You're not giving me a very clear picture of what you'd like me to do
here. :-)  Did you mean "even though" instead of "OTOH"?  ("On the other
hand" usually indicates a change of mind.)

 -George

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

Re: [PATCH] x86/emulate: Don't assume that addr_size == 32 implies protected mode

[Jan Beulich]

>>> George Dunlap <george.dunlap@citrix.com> 12/28/16 2:34 PM >>>
>On 16/12/16 10:34, Jan Beulich wrote:
>>>>> On 16.12.16 at 10:55, <george.dunlap@citrix.com> wrote:
>>> Callers of x86_emulate() generally define addr_size based on the code
>>> segment.  In vm86 mode, the code segment is set by the hardware to be
>>> 16-bits; but it is entirely possible to enable protected mode, set the
>>> CS to 32-bits, and then disable protected mode.  (This is commonly
>>> called "unreal mode".)
>> 
>> To better match this description I think it would be preferable ...
>> 
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -2149,11 +2149,8 @@ x86_decode(
>>>              default:
>>>                  BUG(); /* Shouldn't be possible. */
>>>              case 2:
>>> -                if ( in_realmode(ctxt, ops) || (state->regs->eflags & EFLG_VM) )
>>> -                    break;
>>> -                /* fall through */
>>>              case 4:
>>> -                if ( modrm_mod != 3 )
>>> +                if ( modrm_mod != 3 || !in_protmode(ctxt, ops) )
>>>                      break;
>> 
>> ... to keep the EFLAGS.VM in case 2, and check in_realmode()
>> in case 4. Otoh what you have now is the more compact form,
>> resulting in fewer branches ...
>
>You're not giving me a very clear picture of what you'd like me to do
>here. :-)  Did you mean "even though" instead of "OTOH"?  ("On the other
>hand" usually indicates a change of mind.)

My first thought was to ask you to make that code change. But then I
realized that the code as is has its own advantage, in which case it
would seem desirable to make the description better match that change
(as said, it looks to me as if it rather describes your change including
the suggested adjustment, since as it is still shown above it's basically
also allowing 32-bit VM86 mode).

Bottom line: I'd be fine either way, as long as description and code
change match up.

Jan


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