Re: [PATCH V2 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

["Jan Beulich" <JBeulich@suse.com>]

>>> On 15.06.15 at 11:03, <rcojocaru@bitdefender.com> wrote:
> Deny register writes if a vm_client subscribed to mov_to_msr events
> forbids them. Currently supported for MSR, CR0, CR3 and CR4 events.

Is the first sentence referring the mov_to_msr stale?

> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -90,7 +90,7 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
>      return 1;
>  }
>  
> -void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
> +bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
>  {
>      struct arch_domain *currad = &current->domain->arch;
>      unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
> @@ -109,7 +109,10 @@ void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
>  
>          hvm_event_traps(currad->monitor.write_ctrlreg_sync & ctrlreg_bitmask,
>                          &req);
> +        return 1;
>      }
> +
> +    return 0;
>  }
>  
>  void hvm_event_msr(unsigned int msr, uint64_t value)

Why is knowing whether an event was sent relevant for
hvm_event_cr(), but not for hvm_event_msr()?

> @@ -468,6 +469,37 @@ void hvm_do_resume(struct vcpu *v)
>          }
>      }
>  
> +    ASSERT(v == current);
> +
> +    if ( d->arch.event_write_data )

unlikely() to not penalize the common case? Also an ASSERT() like
this belongs either at the beginning of the function or, if really only
relevant with the changes you do here, inside the if().

> @@ -3189,12 +3221,13 @@ static void hvm_update_cr(struct vcpu *v, unsigned int cr, unsigned long value)
>      hvm_update_guest_cr(v, cr);
>  }
>  
> -int hvm_set_cr0(unsigned long value)
> +int hvm_set_cr0(unsigned long value, bool_t event_only)

"event_only" seems pretty misleading, as it limits the function's
operation only when ...

> @@ -3224,6 +3257,22 @@ int hvm_set_cr0(unsigned long value)
>          goto gpf;
>      }
>  
> +    if ( event_only && unlikely(currad->monitor.write_ctrlreg_enabled &
> +                                monitor_ctrlreg_bitmask(VM_EVENT_X86_CR0)) &&
> +                                value != old_value )

... a number of other conditions are true. Maybe "may_defer" or
some such making clear this is conditional behavior?

Also - indentation.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2010,9 +2010,9 @@ static int vmx_cr_access(unsigned long exit_qualification)
>      }
>      case VMX_CONTROL_REG_ACCESS_TYPE_CLTS: {
>          unsigned long old = curr->arch.hvm_vcpu.guest_cr[0];
> +        hvm_event_crX(CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
>          curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
>          vmx_update_guest_cr(curr, 0);
> -        hvm_event_crX(CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
>          HVMTRACE_0D(CLTS);
>          break;
>      }

I suppose it is intentional to ignore a possible deny here? If so,
shouldn't the be documented by way of a comment?

Also, since you already touch this code, please add a blank line
between declaration and statements.

> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1048,15 +1048,16 @@ static void load_shadow_guest_state(struct vcpu *v)
>  
>      nvcpu->guest_cr[0] = __get_vvmcs(vvmcs, CR0_READ_SHADOW);
>      nvcpu->guest_cr[4] = __get_vvmcs(vvmcs, CR4_READ_SHADOW);
> -    hvm_set_cr0(__get_vvmcs(vvmcs, GUEST_CR0));
> -    hvm_set_cr4(__get_vvmcs(vvmcs, GUEST_CR4));
> -    hvm_set_cr3(__get_vvmcs(vvmcs, GUEST_CR3));
> +    hvm_set_cr0(__get_vvmcs(vvmcs, GUEST_CR0), 1);
> +    hvm_set_cr4(__get_vvmcs(vvmcs, GUEST_CR4), 1);
> +    hvm_set_cr3(__get_vvmcs(vvmcs, GUEST_CR3), 1);

Are you sure there are no dependencies between the individual
registers getting set? And what about the order here versus how
you carry out the deferred writes in hvm_do_resume()? I don't
think any good can come from updating CR3 before CR4...

>      control = __get_vvmcs(vvmcs, VM_ENTRY_CONTROLS);
>      if ( control & VM_ENTRY_LOAD_GUEST_PAT )
>          hvm_set_guest_pat(v, __get_vvmcs(vvmcs, GUEST_PAT));
>      if ( control & VM_ENTRY_LOAD_PERF_GLOBAL_CTRL )
> -        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, __get_vvmcs(vvmcs, GUEST_PERF_GLOBAL_CTRL));
> +        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
> +                                __get_vvmcs(vvmcs, GUEST_PERF_GLOBAL_CTRL), 1);

So one special MSR gets potentially denied writes to - how about
all the other ones like PAT (visible above), EFER, etc? And once
you send events for multiple ones - how would you know which
ones were denied access to be the time to reach hvm_do_resume()?

All the same questions of course apply to nested SVM code too, just
that there the problems are leass easily visible from looking at the
patch.

> @@ -1249,15 +1250,16 @@ static void load_vvmcs_host_state(struct vcpu *v)
>          __vmwrite(vmcs_h2g_field[i].guest_field, r);
>      }
>  
> -    hvm_set_cr0(__get_vvmcs(vvmcs, HOST_CR0));
> -    hvm_set_cr4(__get_vvmcs(vvmcs, HOST_CR4));
> -    hvm_set_cr3(__get_vvmcs(vvmcs, HOST_CR3));
> +    hvm_set_cr0(__get_vvmcs(vvmcs, HOST_CR0), 1);
> +    hvm_set_cr4(__get_vvmcs(vvmcs, HOST_CR4), 1);
> +    hvm_set_cr3(__get_vvmcs(vvmcs, HOST_CR3), 1);
>  
>      control = __get_vvmcs(vvmcs, VM_EXIT_CONTROLS);
>      if ( control & VM_EXIT_LOAD_HOST_PAT )
>          hvm_set_guest_pat(v, __get_vvmcs(vvmcs, HOST_PAT));
>      if ( control & VM_EXIT_LOAD_PERF_GLOBAL_CTRL )
> -        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, __get_vvmcs(vvmcs, HOST_PERF_GLOBAL_CTRL));
> +        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
> +                                __get_vvmcs(vvmcs, HOST_PERF_GLOBAL_CTRL), 1);

Considering these together with the above - do you really want/
need to intercept and send events for both host and guest shadow
state changes?

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1417,6 +1417,35 @@ static void p2m_vm_event_fill_regs(vm_event_request_t *req)
>  void p2m_mem_access_emulate_check(struct vcpu *v,
>                                    const vm_event_response_t *rsp)
>  {
> +    ASSERT(v->domain->arch.event_write_data != NULL);
> +
> +    if ( rsp->flags & MEM_ACCESS_DENY )
> +    {
> +        struct monitor_write_data *w =
> +            &v->domain->arch.event_write_data[v->vcpu_id];
> +

I think the ASSERT() above belongs here.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -250,6 +250,21 @@ struct pv_domain
>      struct mapcache_domain mapcache;
>  };
>  
> +struct monitor_write_data {
> +    struct {
> +        uint8_t msr : 1;
> +        uint8_t cr0 : 1;
> +        uint8_t cr3 : 1;
> +        uint8_t cr4 : 1;

unsigned int

> +    } do_write;
> +
> +    uint64_t msr;

unsigned int or uint32_t

> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -158,6 +158,11 @@ struct vm_event_regs_x86 {
>   * MEM_ACCESS_EMULATE_NOWRITE.
>   */
>  #define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
> + /*
> +  * Deny completion of the operation that triggered the event.
> +  * Currently only useful for MSR write events.
> +  */
> +#define MEM_ACCESS_DENY                 (1 << 9)

Same as for the description - isn't the comment, referring to only
MSR writes, stale?

Jan