Re: [Xen-devel] [PATCH v11 6/9] xen: Add ring 3 vmware_port support

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 05/22/2015 04:50 PM, Don Slutz wrote:
> Summary is that VMware treats "in (%dx),%eax" (or "out %eax,(%dx)")
> to port 0x5658 specially.  Note: since many operations return data
> in EAX, "in (%dx),%eax" is the one to use.  The other lengths like
> "in (%dx),%al" will still do things, only AL part of EAX will be
> changed.  For "out %eax,(%dx)" of all lengths, EAX will remain
> unchanged.
> 
> This instruction is allowed to be used from ring 3.  To
> support this the vmexit for GP needs to be enabled.  I have not
> fully tested that nested HVM is doing the right thing for this.
> 
> Enable no-fault of pio in x86_emulate for VMware port
> 
> Also adjust the emulation registers after doing a VMware
> backdoor operation.
> 
> Add new routine hvm_emulate_one_gp() to be used by the #GP fault
> handler.
> 
> Some of the best info is at:
> 
> https://sites.google.com/site/chitchatvmback/backdoor
> 
> Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>

So let me get this straight.

VMWare allows ring3 to access the magic port regardless of whether the
guest OS has enabled access to that IO port or not.

In order to emulate this, we need to:
* Trap to Xen on #GPs rather than just letting the hardware handle it
* Emulate all instructions which cause a #GP, just to see if they might
be an IO instruction accessing the magic port.
* If it is an IO instruction, and it's accessing the magic port, then we
skip the ioport access checks (which will cause the instruction to
execute as though it had been given access).
* Under all other circumstances (we hope) the emulator in Xen will do
exactly what the hardware just did, and deliver a #GP to the guest.

In an attempt to make this more safe, emulation ops that write (such as
write and cmpxchg) are replaced with stubs which always return an error.

Is that about right?

That sounds completely insane.  It opens up an almost infinite surface
of attack onto the Xen emulator.

I understand that having the "VMWare compatible" is a nice tick-box to
have, but seriously, I cannot imagine that having unprivileged
user-space tools know the real clock frequency without having to involve
the OS is anywhere close to worth the risk involved.

 -George



> ---
> v11:
>   No change
> 
> v10:
>    Re-worked to be simpler.
> 
> v9:
>    Split #GP handling (or skipping of #GP) code out of previous
>    patch to help with the review process.
>    Switch to x86_emulator to handle #GP
>    I think the hvm_emulate_ops_gp() covers all needed ops.  Not able to 
> validate
>    all paths though _hvm_emulate_one().
> 
>  xen/arch/x86/hvm/emulate.c             | 54 
> ++++++++++++++++++++++++++++++++--
>  xen/arch/x86/hvm/svm/svm.c             | 26 ++++++++++++++++
>  xen/arch/x86/hvm/svm/vmcb.c            |  2 ++
>  xen/arch/x86/hvm/vmware/vmport.c       | 11 +++++++
>  xen/arch/x86/hvm/vmx/vmcs.c            |  2 ++
>  xen/arch/x86/hvm/vmx/vmx.c             | 37 +++++++++++++++++++++++
>  xen/arch/x86/x86_emulate/x86_emulate.c | 13 +++++++-
>  xen/arch/x86/x86_emulate/x86_emulate.h |  5 ++++
>  xen/include/asm-x86/hvm/emulate.h      |  2 ++
>  xen/include/asm-x86/hvm/hvm.h          |  1 +
>  10 files changed, 150 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index ac9c9d6..d5e6468 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -803,6 +803,27 @@ static int hvmemul_wbinvd_discard(
>      return X86EMUL_OKAY;
>  }
>  
> +static int hvmemul_write_gp(
> +    unsigned int seg,
> +    unsigned long offset,
> +    void *p_data,
> +    unsigned int bytes,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    return X86EMUL_EXCEPTION;
> +}
> +
> +static int hvmemul_cmpxchg_gp(
> +    unsigned int seg,
> +    unsigned long offset,
> +    void *old,
> +    void *new,
> +    unsigned int bytes,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    return X86EMUL_EXCEPTION;
> +}
> +
>  static int hvmemul_cmpxchg(
>      enum x86_segment seg,
>      unsigned long offset,
> @@ -1356,6 +1377,13 @@ static int hvmemul_invlpg(
>      return rc;
>  }
>  
> +static int hvmemul_vmport_check(
> +    unsigned int first_port,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    return vmport_check_port(first_port);
> +}
> +
>  static const struct x86_emulate_ops hvm_emulate_ops = {
>      .read          = hvmemul_read,
>      .insn_fetch    = hvmemul_insn_fetch,
> @@ -1379,7 +1407,8 @@ static const struct x86_emulate_ops hvm_emulate_ops = {
>      .inject_sw_interrupt = hvmemul_inject_sw_interrupt,
>      .get_fpu       = hvmemul_get_fpu,
>      .put_fpu       = hvmemul_put_fpu,
> -    .invlpg        = hvmemul_invlpg
> +    .invlpg        = hvmemul_invlpg,
> +    .vmport_check  = hvmemul_vmport_check,
>  };
>  
>  static const struct x86_emulate_ops hvm_emulate_ops_no_write = {
> @@ -1405,7 +1434,22 @@ static const struct x86_emulate_ops 
> hvm_emulate_ops_no_write = {
>      .inject_sw_interrupt = hvmemul_inject_sw_interrupt,
>      .get_fpu       = hvmemul_get_fpu,
>      .put_fpu       = hvmemul_put_fpu,
> -    .invlpg        = hvmemul_invlpg
> +    .invlpg        = hvmemul_invlpg,
> +    .vmport_check  = hvmemul_vmport_check,
> +};
> +
> +static const struct x86_emulate_ops hvm_emulate_ops_gp = {
> +    .read          = hvmemul_read,
> +    .insn_fetch    = hvmemul_insn_fetch,
> +    .write         = hvmemul_write_gp,
> +    .cmpxchg       = hvmemul_cmpxchg_gp,
> +    .read_segment  = hvmemul_read_segment,
> +    .write_segment = hvmemul_write_segment,
> +    .read_io       = hvmemul_read_io,
> +    .write_io      = hvmemul_write_io,
> +    .inject_hw_exception = hvmemul_inject_hw_exception,
> +    .inject_sw_interrupt = hvmemul_inject_sw_interrupt,
> +    .vmport_check  = hvmemul_vmport_check,
>  };
>  
>  static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
> @@ -1522,6 +1566,12 @@ int hvm_emulate_one(
>      return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops);
>  }
>  
> +int hvm_emulate_one_gp(
> +    struct hvm_emulate_ctxt *hvmemul_ctxt)
> +{
> +    return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops_gp);
> +}
> +
>  int hvm_emulate_one_no_write(
>      struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 6734fb6..62baf3c 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2119,6 +2119,28 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
>      return;
>  }
>  
> +static void svm_vmexit_gp_intercept(struct cpu_user_regs *regs,
> +                                    struct vcpu *v)
> +{
> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> +    int rc;
> +
> +    if ( vmcb->exitinfo1 != 0 || vmcb->exitinfo2 != 0 )
> +        rc = X86EMUL_EXCEPTION;
> +    else
> +    {
> +        struct hvm_emulate_ctxt ctxt;
> +
> +        hvm_emulate_prepare(&ctxt, regs);
> +        rc = hvm_emulate_one_gp(&ctxt);
> +
> +        if ( rc == X86EMUL_OKAY )
> +            hvm_emulate_writeback(&ctxt);
> +    }
> +    if ( rc != X86EMUL_OKAY && rc != X86EMUL_RETRY )
> +        hvm_inject_hw_exception(TRAP_gp_fault, vmcb->exitinfo1);
> +}
> +
>  static void svm_vmexit_ud_intercept(struct cpu_user_regs *regs)
>  {
>      struct hvm_emulate_ctxt ctxt;
> @@ -2484,6 +2506,10 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>      }
>  
> +    case VMEXIT_EXCEPTION_GP:
> +        svm_vmexit_gp_intercept(regs, v);
> +        break;
> +
>      case VMEXIT_EXCEPTION_UD:
>          svm_vmexit_ud_intercept(regs);
>          break;
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 6339d2a..7683c09 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -195,6 +195,8 @@ static int construct_vmcb(struct vcpu *v)
>          HVM_TRAP_MASK
>          | (1U << TRAP_no_device);
>  
> +    if ( v->domain->arch.hvm_domain.is_vmware_port_enabled )
> +        vmcb->_exception_intercepts |= 1U << TRAP_gp_fault;
>      if ( paging_mode_hap(v->domain) )
>      {
>          vmcb->_np_enable = 1; /* enable nested paging */
> diff --git a/xen/arch/x86/hvm/vmware/vmport.c 
> b/xen/arch/x86/hvm/vmware/vmport.c
> index f24d8e3..36e3f1b 100644
> --- a/xen/arch/x86/hvm/vmware/vmport.c
> +++ b/xen/arch/x86/hvm/vmware/vmport.c
> @@ -137,6 +137,17 @@ void vmport_register(struct domain *d)
>      register_portio_handler(d, BDOOR_PORT, 4, vmport_ioport);
>  }
>  
> +int vmport_check_port(unsigned int port)
> +{
> +    struct vcpu *curr = current;
> +    struct domain *currd = curr->domain;
> +
> +    if ( port == BDOOR_PORT && is_hvm_domain(currd) &&
> +         currd->arch.hvm_domain.is_vmware_port_enabled )
> +        return 0;
> +    return 1;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 877ec10..54360b0 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1151,6 +1151,8 @@ static int construct_vmcs(struct vcpu *v)
>  
>      v->arch.hvm_vmx.exception_bitmap = HVM_TRAP_MASK
>                | (paging_mode_hap(d) ? 0 : (1U << TRAP_page_fault))
> +              | (v->domain->arch.hvm_domain.is_vmware_port_enabled ?
> +                 (1U << TRAP_gp_fault) : 0)
>                | (1U << TRAP_no_device);
>      vmx_update_exception_bitmap(v);
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 74f563f..fe88afe 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1312,6 +1312,8 @@ static void vmx_update_guest_cr(struct vcpu *v, 
> unsigned int cr)
>                  v->arch.hvm_vmx.exception_bitmap = HVM_TRAP_MASK
>                            | (paging_mode_hap(v->domain) ?
>                               0 : (1U << TRAP_page_fault))
> +                          | 
> (v->domain->arch.hvm_domain.is_vmware_port_enabled ?
> +                             (1U << TRAP_gp_fault) : 0)
>                            | (1U << TRAP_no_device);
>                  vmx_update_exception_bitmap(v);
>                  vmx_update_debug_state(v);
> @@ -2671,6 +2673,38 @@ static void vmx_idtv_reinject(unsigned long idtv_info)
>      }
>  }
>  
> +static void vmx_vmexit_gp_intercept(struct cpu_user_regs *regs,
> +                                    struct vcpu *v)
> +{
> +    unsigned long exit_qualification;
> +    unsigned long ecode;
> +    int rc;
> +    unsigned long vector;
> +
> +    __vmread(VM_EXIT_INTR_INFO, &vector);
> +    ASSERT(vector & INTR_INFO_VALID_MASK);
> +    ASSERT(vector & INTR_INFO_DELIVER_CODE_MASK);
> +
> +    __vmread(EXIT_QUALIFICATION, &exit_qualification);
> +    __vmread(VM_EXIT_INTR_ERROR_CODE, &ecode);
> +
> +    if ( ecode != 0 || exit_qualification != 0 )
> +        rc = X86EMUL_EXCEPTION;
> +    else
> +    {
> +        struct hvm_emulate_ctxt ctxt;
> +
> +        hvm_emulate_prepare(&ctxt, regs);
> +        rc = hvm_emulate_one_gp(&ctxt);
> +
> +        if ( rc == X86EMUL_OKAY )
> +            hvm_emulate_writeback(&ctxt);
> +    }
> +
> +    if ( rc != X86EMUL_OKAY && rc != X86EMUL_RETRY )
> +        hvm_inject_hw_exception(TRAP_gp_fault, ecode);
> +}
> +
>  static int vmx_handle_apic_write(void)
>  {
>      unsigned long exit_qualification;
> @@ -2895,6 +2929,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              HVMTRACE_1D(TRAP, vector);
>              vmx_fpu_dirty_intercept();
>              break;
> +        case TRAP_gp_fault:
> +            vmx_vmexit_gp_intercept(regs, v);
> +            break;
>          case TRAP_page_fault:
>              __vmread(EXIT_QUALIFICATION, &exit_qualification);
>              __vmread(VM_EXIT_INTR_ERROR_CODE, &ecode);
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c 
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index c017c69..d3ea143 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -3393,8 +3393,11 @@ x86_emulate(
>          unsigned int port = ((b < 0xe8)
>                               ? insn_fetch_type(uint8_t)
>                               : (uint16_t)_regs.edx);
> +     bool_t vmport = (ops->vmport_check && /* Vmware backdoor? */
> +                      (ops->vmport_check(port, ctxt) == 0));
>          op_bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
> -        if ( (rc = ioport_access_check(port, op_bytes, ctxt, ops)) != 0 )
> +        if ( !vmport &&
> +          (rc = ioport_access_check(port, op_bytes, ctxt, ops)) != 0 )
>              goto done;
>          if ( b & 2 )
>          {
> @@ -3413,6 +3416,14 @@ x86_emulate(
>          }
>          if ( rc != 0 )
>              goto done;
> +     if ( vmport )
> +     {
> +            _regs._ebx = ctxt->regs->_ebx;
> +            _regs._ecx = ctxt->regs->_ecx;
> +            _regs._edx = ctxt->regs->_edx;
> +            _regs._esi = ctxt->regs->_esi;
> +            _regs._edi = ctxt->regs->_edi;
> +     }
>          break;
>      }
>  
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h 
> b/xen/arch/x86/x86_emulate/x86_emulate.h
> index 064b8f4..d914b5e 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -397,6 +397,11 @@ struct x86_emulate_ops
>          enum x86_segment seg,
>          unsigned long offset,
>          struct x86_emulate_ctxt *ctxt);
> +
> +    /* vmport_check */
> +    int (*vmport_check)(
> +        unsigned int port,
> +        struct x86_emulate_ctxt *ctxt);
>  };
>  
>  struct cpu_user_regs;
> diff --git a/xen/include/asm-x86/hvm/emulate.h 
> b/xen/include/asm-x86/hvm/emulate.h
> index b3971c8..4386169 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -36,6 +36,8 @@ struct hvm_emulate_ctxt {
>  
>  int hvm_emulate_one(
>      struct hvm_emulate_ctxt *hvmemul_ctxt);
> +int hvm_emulate_one_gp(
> +    struct hvm_emulate_ctxt *hvmemul_ctxt);
>  int hvm_emulate_one_no_write(
>      struct hvm_emulate_ctxt *hvmemul_ctxt);
>  void hvm_mem_access_emulate_one(bool_t nowrite,
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index e76f612..c42f7d8 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -523,6 +523,7 @@ extern bool_t opt_hvm_fep;
>  #endif
>  
>  void vmport_register(struct domain *d);
> +int vmport_check_port(unsigned int port);
>  
>  #endif /* __ASM_X86_HVM_HVM_H__ */
>  
> 


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