| Message ID | 1481882103-18332-1-git-send-email-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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
>>> 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
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:
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(-)