Re: [PATCH RFC V2 4/6] xen: Support for VMCALL mem_events

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

On 11/07/14 16:43, 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).
>
> Changes since V1:
>  - Added a #define and an comment explaining a previous magic
>    constant.
>  - Had MEM_EVENT_REASON_VMCALL explicitly not honour
>    HVMPME_onchangeonly.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  xen/arch/x86/hvm/hvm.c          |    9 +++++++++
>  xen/arch/x86/hvm/vmx/vmx.c      |   18 +++++++++++++++++-
>  xen/include/asm-x86/hvm/hvm.h   |    1 +
>  xen/include/public/hvm/params.h |    4 +++-
>  xen/include/public/mem_event.h  |    5 +++++
>  5 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 89a0382..6e86d7c 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5564,6 +5564,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              case HVM_PARAM_MEMORY_EVENT_INT3:
>              case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
>              case HVM_PARAM_MEMORY_EVENT_MSR:
> +            case HVM_PARAM_MEMORY_EVENT_VMCALL:
>                  if ( d == current->domain )
>                  {
>                      rc = -EPERM;
> @@ -6199,6 +6200,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 2caa04a..6c63225 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2879,8 +2879,24 @@ 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);
> +
> +        /* Don't send a VMCALL mem_event unless something
> +         * caused the guests's eax register to contain the
> +         * VMCALL_EVENT_REQUEST constant. */
> +        if ( regs->eax != VMCALL_EVENT_REQUEST )
> +        {
> +            rc = hvm_do_hypercall(regs);
> +        }
> +        else
> +        {
> +            hvm_memory_event_vmcall(guest_cpu_user_regs()->eip, eax);
> +            update_guest_eip();
> +            break;
> +        }

Thinking more about this, it is really a hypercall pretending not to
be.  It would be better to introduce a real HVMOP_send_mem_event.

>From the point of view of your in-guest agent, it would be a vmcall with
rax = 34 (hvmop) rdi = $N (send_mem_event subop) rsi = data or pointer
to struct containing data, depending on how exactly you implement the
hypercall.

You would have the bonus of being able to detect errors, e.g. -ENOENT
for "mem_event not active", get SVM support for free, and not need magic
numbers, or vendor specific terms like "vmcall" finding their way into
the Xen public API.

> +
>          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 0ebd478..3c0af30 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -475,6 +475,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 614ff5f..d8f89b5 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -151,6 +151,8 @@
>  /* Location of the VM Generation ID in guest physical address space. */
>  #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
>  
> -#define HVM_NR_PARAMS          35
> +#define HVM_PARAM_MEMORY_EVENT_VMCALL 35

What is this hvmparam actually used for?  This patch only reads it, and
as indicated previously, it is readwrite to the guest which likely
breaks any assumptions you have about the trustworthness of the value
found there.

~Andrew

> +
> +#define HVM_NR_PARAMS          36
>  
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
> diff --git a/xen/include/public/mem_event.h b/xen/include/public/mem_event.h
> index b9af728..7a59083 100644
> --- a/xen/include/public/mem_event.h
> +++ b/xen/include/public/mem_event.h
> @@ -47,6 +47,11 @@
>  #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;
> +                                             does NOT honour HVMPME_onchangeonly */
> +
> +/* VMCALL mem_events will only be sent when the guest's EAX holds this value. */
> +#define VMCALL_EVENT_REQUEST 0x494E5452 /* 'INTR' */
>  
>  /* Using a custom struct (not hvm_hw_cpu) so as to not fill
>   * the mem_event ring buffer too quickly. */