Re: [PATCH RFC 5/9] xen: Support for VMCALL mem_events

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 02/07/14 14:33, Razvan Cojocaru wrote:
> Added support for VMCALL events (the memory introspection library
> will have the guest trigger VMCALLs, which will then be sent along
> via the mem_event mechanism).
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Am I correct in concluding that this is an escape mechanism for
something inside the guest to trap to the toolstack userspace monitoring
the guest?

> ---
>  xen/arch/x86/hvm/hvm.c          |    8 ++++++++
>  xen/arch/x86/hvm/vmx/vmx.c      |   15 ++++++++++++++-
>  xen/include/asm-x86/hvm/hvm.h   |    1 +
>  xen/include/public/hvm/params.h |    4 +++-
>  xen/include/public/mem_event.h  |    1 +
>  5 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index f65a5f5..df696d1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6130,6 +6130,14 @@ void hvm_memory_event_msr(unsigned long msr, unsigned long value)
>                             value, ~value, 1, msr);
>  }
>  
> +void hvm_memory_event_vmcall(unsigned long rip, unsigned long eax)
> +{
> +    hvm_memory_event_traps(current->domain->arch.hvm_domain
> +                             .params[HVM_PARAM_MEMORY_EVENT_VMCALL],
> +                           MEM_EVENT_REASON_VMCALL,
> +                           rip, ~rip, 1, eax);
> +}
> +
>  int hvm_memory_event_int3(unsigned long gla) 
>  {
>      uint32_t pfec = PFEC_page_present;
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index fed21b6..b4c12cd 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2880,8 +2880,21 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>      case EXIT_REASON_VMCALL:
>      {
>          int rc;
> +        unsigned long eax = regs->eax;
> +
>          HVMTRACE_1D(VMMCALL, regs->eax);
> -        rc = hvm_do_hypercall(regs);
> +
> +        if ( regs->eax != 0x494e5452 ) /* Introcore magic */

This needs to live somewhere in the public API.

> +        {
> +            rc = hvm_do_hypercall(regs);
> +        }
> +        else
> +        {
> +            hvm_memory_event_vmcall(guest_cpu_user_regs()->eip, eax);
> +            update_guest_eip();
> +            break;
> +        }
> +
>          if ( rc != HVM_HCALL_preempted )
>          {
>              update_guest_eip(); /* Safe: VMCALL */
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 90e69f5..67e365b 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -532,6 +532,7 @@ void hvm_memory_event_cr0(unsigned long value, unsigned long old);
>  void hvm_memory_event_cr3(unsigned long value, unsigned long old);
>  void hvm_memory_event_cr4(unsigned long value, unsigned long old);
>  void hvm_memory_event_msr(unsigned long msr, unsigned long value);
> +void hvm_memory_event_vmcall(unsigned long rip, unsigned long eax);
>  /* Called for current VCPU on int3: returns -1 if no listener */
>  int hvm_memory_event_int3(unsigned long gla);
>  
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index f830bdd..ea2eee6 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -148,6 +148,8 @@
>  #define HVM_PARAM_IOREQ_SERVER_PFN 32
>  #define HVM_PARAM_NR_IOREQ_SERVER_PAGES 33
>  
> -#define HVM_NR_PARAMS          34
> +#define HVM_PARAM_MEMORY_EVENT_VMCALL 34
> +
> +#define HVM_NR_PARAMS          35

Nothing prevents the VM from writing whatever value it wishes into this
hvmparam using the setparam hypercall, which would look to stump dom0
userspace trusting the value it finds there.

The current infrastructure for hvmparams is far too lax and I have half
a patch series (which is distinctly low down my todo list in terms of
priority) which tries to tighten the restrictions.  However in the
meantime all new params should have proper restrictions applied.

Having said that, this would appear to interact badly with mem_events in
PV guests, which have recently (or are planning to?) moved away from
being HVM specific.  It would be preferable not to impede that.

~Andrew

>  
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
> diff --git a/xen/include/public/mem_event.h b/xen/include/public/mem_event.h
> index 24ac67d..5fa2858 100644
> --- a/xen/include/public/mem_event.h
> +++ b/xen/include/public/mem_event.h
> @@ -47,6 +47,7 @@
>  #define MEM_EVENT_REASON_SINGLESTEP  6    /* single step was invoked: gla/gfn are RIP */
>  #define MEM_EVENT_REASON_MSR         7    /* MSR was hit: gfn is MSR value, gla is MSR address;
>                                               does NOT honour HVMPME_onchangeonly */
> +#define MEM_EVENT_REASON_VMCALL      8    /* VMCALL: gfn is RIP, gla is EAX */
>  
>  typedef struct mem_event_regs_st {
>      uint64_t rax;