Re: [PATCH v3 4/8] x86/vm_event/monitor/cr: check for vm-event subscriber on domctl

[Corneliu ZUZU <czuzu@bitdefender.com>]

On 7/6/2016 6:52 PM, Corneliu ZUZU wrote:
> Enforce presence of a monitor vm-event subscriber when the toolstack user calls
> xc_monitor_write_ctrlreg (XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG domctl).
> Without this change, ASSERT(v->arch.vm_event) @ hvm_set_cr0 and such would fail
> if the toolstack user calls xc_monitor_write_ctrlreg(...) w/ enable = true,
> without first calling xc_monitor_enable().
>
> Also adjust returned error code for similar check from -EINVAL to more
> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
>   xen/arch/x86/monitor.c        | 7 +++++++
>   xen/include/asm-x86/monitor.h | 2 +-
>   2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 42f31bf..4d4db33 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -188,6 +188,13 @@ int arch_monitor_domctl_event(struct domain *d,
>           unsigned int ctrlreg_bitmask;
>           bool_t old_status;
>   
> +        /*
> +         * Enabling cr-write vm-events without a vm_event subscriber is
> +         * meaningless.
> +         */
> +        if ( !vm_event_domain_initialised(d) )
> +            return -ENOSYS;
> +
>           /* sanity check: avoid left-shift undefined behavior */
>           if ( unlikely(mop->u.mov_to_cr.index > 31) )
>               return -EINVAL;
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 9238ec8..2213124 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -52,7 +52,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>           if ( vm_event_domain_initialised(d) )
>               d->arch.mem_access_emulate_each_rep = !!mop->event;
>           else
> -            rc = -EINVAL;
> +            rc = -ENOSYS;
>   
>           domain_unpause(d);
>           break;

But this also doesn't guarantee the ASSERT not failing, as I now 
realize, simply because with this patch v->arch.vm_event can *still be 
NULL* even with a *non-zero d->monitor.write_ctrlreg_enabled*.

Can happen for example if:
1) the toolstack user calls *xc_monitor_enable()* to enable domain 
monitoring (calls *vm_event_init_domain()* along the way)
2) the toolstack user calls *xc_monitor_write_ctrlreg()* to enable *CR0* 
exiting - this will succeed since the user *first called 
xc_monitor_enable()* - which *will result in having a non-zero 
d->monitor.write_ctrlreg_enabled*
3) the toolstack user calls *xc_mem_paging_enable() followed by 
xc_mem_paging_disable()*; this *will call 
vm_event_disable()->vm_event_cleanup_domain()* along the way which *will 
xfree v->arch.vm_event*, but _will not_ call 
*arch_monitor_cleanup_domain()* (see vm_event_domctl) which zeroes out 
d->monitor.write_ctrlreg_enabled
4) a CR0-write is intercepted after these operations which will 
ASSERT(v->arch.vm_event) in hvm_set_cr0(), which will subsequently fail

This issue happens because *vm_event_cleanup()* domain *xfrees 
arch_vm_event along with its write_data field* needed by the monitor 
subsystem, even a vm-event subsystem other than the monitor one is 
uninitialized. I'm wondering why vm_event_{init,cleanup}_domain() 
allocate/free *monitor-specific* resources (arch_vm_event fields) even 
when they're called as a result of 
XEN_DOMCTL_VM_EVENT_OP_PAGING/XEN_DOMCTL_VM_EVENT_OP_SHARING domctl.

Tamas, Razvan, this seems wrong, shouldn't most operations currently 
done in *vm_event_{init,cleanup}_domain* actually be done in 
*arch_monitor_{init,cleanup}_domain* instead?

I propose the following refactoring:

-  define:

         struct arch_vm_event_monitor {
             uint32_t emulate_flags;
             struct vm_event_emul_read_data *emul_read_data;
             struct monitor_write_data write_data;
         }

- change

         struct arch_vm_event {
             struct arch_vm_event_monitor *monitor; // monitor 
subsystem-stuff!
         }

- allocate *arch_vm_event* (but don't touch its *monitor* field) in 
*vm_event_init_domain()*

- allocate/free *arch_vm_event->monitor* in 
*arch_monitor_{init,cleanup}_domain()*, but *only free write_data if 
there are no uncommitted writes*

- call arch_monitor_cleanup_domain() *before* vm_event_cleanup_domain() 
and only free arch_vm_event in the latter if arch_vm_event->monitor was 
freed in the former

- on domain/vcpu destroyal free everything left allocated 
(arch_vm_event->monitor and arch_vm_event)

Sounds good?

Thanks,
Corneliu.

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