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 <cz...@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
