diff mbox

[v12,4/4] x86emul: Raise #UD when emulating an unrecognized instruction.

Message ID 1505970726-5671-5-git-send-email-ppircalabu@bitdefender.com (mailing list archive)
State New, archived
Headers show

Commit Message

Petre Ovidiu PIRCALABU Sept. 21, 2017, 5:12 a.m. UTC 
Modified the behavior of hvm_emulate_one_insn and
vmx_realmode_emulate_one to generate an Invalid Opcode trap when
X86EMUL_UNRECOGNIZED is returned by the emulator instead of just
crashing the domain.

Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/io.c           |  6 +++++-
 xen/arch/x86/hvm/vmx/realmode.c | 11 ++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

Comments

Paul Durrant Sept. 21, 2017, 8:57 a.m. UTC | #1 
> -----Original Message-----
> From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com]
> Sent: 21 September 2017 06:12
> To: xen-devel@lists.xen.org
> Cc: Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; jbeulich@suse.com; konrad.wilk@oracle.com;
> sstabellini@kernel.org; Tim (Xen.org) <tim@xen.org>; Paul Durrant
> <Paul.Durrant@citrix.com>; rcojocaru@bitdefender.com;
> tamas@tklengyel.com; jun.nakajima@intel.com; Kevin Tian
> <kevin.tian@intel.com>; Petre Pircalabu <ppircalabu@bitdefender.com>
> Subject: [PATCH v12 4/4] x86emul: Raise #UD when emulating an
> unrecognized instruction.
> 
> Modified the behavior of hvm_emulate_one_insn and
> vmx_realmode_emulate_one to generate an Invalid Opcode trap when
> X86EMUL_UNRECOGNIZED is returned by the emulator instead of just
> crashing the domain.
> 
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
>  xen/arch/x86/hvm/io.c           |  6 +++++-
>  xen/arch/x86/hvm/vmx/realmode.c | 11 ++++++++++-
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index 7152c28..c7b1c53 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -96,10 +96,14 @@ bool
> hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char
> *descr)
>      switch ( rc )
>      {
>      case X86EMUL_UNHANDLEABLE:
> -    case X86EMUL_UNIMPLEMENTED:
>          hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
>          return false;
> 
> +    case X86EMUL_UNRECOGNIZED:
> +        hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
> +        hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
> +        break;
> +
>      case X86EMUL_EXCEPTION:
>          hvm_inject_event(&ctxt.ctxt.event);
>          break;
> diff --git a/xen/arch/x86/hvm/vmx/realmode.c
> b/xen/arch/x86/hvm/vmx/realmode.c
> index b93792d..03dea6c 100644
> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -106,12 +106,21 @@ void vmx_realmode_emulate_one(struct
> hvm_emulate_ctxt *hvmemul_ctxt)
>      if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry )
>          vio->io_completion = HVMIO_realmode_completion;
> 
> -    if ( rc == X86EMUL_UNHANDLEABLE || rc == X86EMUL_UNIMPLEMENTED
> )
> +    if ( rc == X86EMUL_UNHANDLEABLE )

I don't quite understand this change. Why has it become unnecessary to deal with X86EMUL_UNIMPLEMENTED? Patch #1 added this change so it seems odd that patch #4 would then revert it.

  Paul

>      {
>          gdprintk(XENLOG_ERR, "Failed to emulate insn.\n");
>          goto fail;
>      }
> 
> +    if ( rc == X86EMUL_UNRECOGNIZED )
> +    {
> +        gdprintk(XENLOG_ERR, "Unrecognized insn.\n");
> +        if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE )
> +            goto fail;
> +
> +        realmode_deliver_exception(TRAP_invalid_op, 0, hvmemul_ctxt);
> +    }
> +
>      if ( rc == X86EMUL_EXCEPTION )
>      {
>          if ( unlikely(curr->domain->debugger_attached) &&
> --
> 2.7.4
Jan Beulich Sept. 21, 2017, 12:44 p.m. UTC | #2 
>>> On 21.09.17 at 10:57, <Paul.Durrant@citrix.com> wrote:
>> From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com]
>> Sent: 21 September 2017 06:12
>> --- a/xen/arch/x86/hvm/vmx/realmode.c
>> +++ b/xen/arch/x86/hvm/vmx/realmode.c
>> @@ -106,12 +106,21 @@ void vmx_realmode_emulate_one(struct
>> hvm_emulate_ctxt *hvmemul_ctxt)
>>      if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry )
>>          vio->io_completion = HVMIO_realmode_completion;
>> 
>> -    if ( rc == X86EMUL_UNHANDLEABLE || rc == X86EMUL_UNIMPLEMENTED
>> )
>> +    if ( rc == X86EMUL_UNHANDLEABLE )
> 
> I don't quite understand this change. Why has it become unnecessary to deal 
> with X86EMUL_UNIMPLEMENTED? Patch #1 added this change so it seems odd that 
> patch #4 would then revert it.

Yeah, it would certainly be more natural to bring things into
their final shape right away.

Jan
Petre Ovidiu PIRCALABU Sept. 25, 2017, 6:22 a.m. UTC | #3 
On Jo, 2017-09-21 at 06:44 -0600, Jan Beulich wrote:
> >

> > >

> > > >

> > > > On 21.09.17 at 10:57, <Paul.Durrant@citrix.com> wrote:

> > > From: Petre Pircalabu [mailto:ppircalabu@bitdefender.com]

> > > Sent: 21 September 2017 06:12

> > > --- a/xen/arch/x86/hvm/vmx/realmode.c

> > > +++ b/xen/arch/x86/hvm/vmx/realmode.c

> > > @@ -106,12 +106,21 @@ void vmx_realmode_emulate_one(struct

> > > hvm_emulate_ctxt *hvmemul_ctxt)

> > >      if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry )

> > >          vio->io_completion = HVMIO_realmode_completion;

> > >

> > > -    if ( rc == X86EMUL_UNHANDLEABLE || rc ==

> > > X86EMUL_UNIMPLEMENTED

> > > )

> > > +    if ( rc == X86EMUL_UNHANDLEABLE )

> > I don't quite understand this change. Why has it become unnecessary

> > to deal

> > with X86EMUL_UNIMPLEMENTED? Patch #1 added this change so it seems

> > odd that

> > patch #4 would then revert it.

> Yeah, it would certainly be more natural to bring things into

> their final shape right away.

>

> Jan

>

>

> ________________________

> This email was scanned by Bitdefender

Thank-you very much for your observation.
I will squash this patch into the first patch of the series. ("x86emul:
New return code for unimplemented instruction")

//Petre

________________________
This email was scanned by Bitdefender
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 7152c28..c7b1c53 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -96,10 +96,14 @@  bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr)
     switch ( rc )
     {
     case X86EMUL_UNHANDLEABLE:
-    case X86EMUL_UNIMPLEMENTED:
         hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
         return false;
 
+    case X86EMUL_UNRECOGNIZED:
+        hvm_dump_emulation_state(XENLOG_G_WARNING, descr, &ctxt, rc);
+        hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+        break;
+
     case X86EMUL_EXCEPTION:
         hvm_inject_event(&ctxt.ctxt.event);
         break;
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index b93792d..03dea6c 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -106,12 +106,21 @@  void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
     if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry )
         vio->io_completion = HVMIO_realmode_completion;
 
-    if ( rc == X86EMUL_UNHANDLEABLE || rc == X86EMUL_UNIMPLEMENTED )
+    if ( rc == X86EMUL_UNHANDLEABLE )
     {
         gdprintk(XENLOG_ERR, "Failed to emulate insn.\n");
         goto fail;
     }
 
+    if ( rc == X86EMUL_UNRECOGNIZED )
+    {
+        gdprintk(XENLOG_ERR, "Unrecognized insn.\n");
+        if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE )
+            goto fail;
+
+        realmode_deliver_exception(TRAP_invalid_op, 0, hvmemul_ctxt);
+    }
+
     if ( rc == X86EMUL_EXCEPTION )
     {
         if ( unlikely(curr->domain->debugger_attached) &&