[PATCH 0/2] x86emul: deal with ASSERT()s triggering

[PATCH 0/2] x86emul: deal with ASSERT()s triggering

[Jan Beulich]

1: x86emul: correct VEX/XOP/EVEX operand size handling for 16-bit code
2: x86/emulate: don't assume that addr_size == 32 implies protected mode

Signed-off-by: Jan Beulich <jbeulich@suse.com>


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

[PATCH 1/2] x86emul: deal with ASSERT()s triggering

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

Operand size defaults to 32 bits in that case, but would not have been
set that way in the absence of an operand size override.

Reported-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2298,6 +2298,11 @@ x86_decode(
             case 8:
                 /* VEX / XOP / EVEX */
                 generate_exception_if(rex_prefix || vex.pfx, EXC_UD);
+                /*
+                 * With operand size override disallowed (see above), op_bytes
+                 * should not have changed from its default.
+                 */
+                ASSERT(op_bytes == def_op_bytes);
 
                 vex.raw[0] = modrm;
                 if ( b == 0xc5 )
@@ -2326,7 +2331,8 @@ x86_decode(
                     }
                     else
                     {
-                        ASSERT(op_bytes == 4);
+                        /* Operand size fixed at 4 (no override via W bit). */
+                        op_bytes = 4;
                         vex.b = 1;
                     }
                     switch ( b )




[-- Attachment #2: x86emul-VEX-16bit.patch --]
[-- Type: text/plain, Size: 1251 bytes --]

x86emul: correct VEX/XOP/EVEX operand size handling for 16-bit code

Operand size defaults to 32 bits in that case, but would not have been
set that way in the absence of an operand size override.

Reported-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2298,6 +2298,11 @@ x86_decode(
             case 8:
                 /* VEX / XOP / EVEX */
                 generate_exception_if(rex_prefix || vex.pfx, EXC_UD);
+                /*
+                 * With operand size override disallowed (see above), op_bytes
+                 * should not have changed from its default.
+                 */
+                ASSERT(op_bytes == def_op_bytes);
 
                 vex.raw[0] = modrm;
                 if ( b == 0xc5 )
@@ -2326,7 +2331,8 @@ x86_decode(
                     }
                     else
                     {
-                        ASSERT(op_bytes == 4);
+                        /* Operand size fixed at 4 (no override via W bit). */
+                        op_bytes = 4;
                         vex.b = 1;
                     }
                     switch ( b )

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

From: George Dunlap <george.dunlap@citrix.com>

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>

Split real mode and VM86 mode handling, as VM86 mode is strictly 16-bit
at all times. Re-base.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

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




[-- Attachment #2: x86emul-unreal-mode.patch --]
[-- Type: text/plain, Size: 1836 bytes --]

x86/emulate: don't assume that addr_size == 32 implies protected mode

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>

Split real mode and VM86 mode handling, as VM86 mode is strictly 16-bit
at all times. Re-base.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/2] x86emul: deal with ASSERT()s triggering

[Andrew Cooper]

On 25/01/17 10:31, Jan Beulich wrote:
> Operand size defaults to 32 bits in that case, but would not have been
> set that way in the absence of an operand size override.
>
> Reported-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Possibly worth mentioning that it was found by fuzzing using AFL?

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

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

[Andrew Cooper]

On 25/01/17 10:32, Jan Beulich wrote:
> From: George Dunlap <george.dunlap@citrix.com>
>
> 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>
>
> Split real mode and VM86 mode handling, as VM86 mode is strictly 16-bit
> at all times. Re-base.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although you
might want to remove the in_protmode() paragraph from the commit message
as it is no longer applicable.

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