| Message ID | 1453989160-8002-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/01/16 13:52, Razvan Cojocaru wrote: > This patch pauses the domain for all writes through the 'ad' > pointer in monitor_domctl(), defers a domain_unpause() call until > after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG > case, and makes sure that the domain is paused for both vm_event > enable and disable cases in vm_event_domctl(). > Thanks go to Andrew Cooper for his review and suggestions. > > Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> Would you mind annotating each of the checks for d != current->domain with /* no domain_pause(). */, which is our normal practice. I think it might be better to move the current check for XEN_DOMCTL_monitor_op into monitor_domctl() itself, to have all checks together in one place. > @@ -144,6 +146,8 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > if ( rc ) > return rc; > > + domain_pause(d); > + > if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && > mop->u.mov_to_msr.extended_capture ) > { > @@ -154,7 +158,6 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) You will need to rework the return -EOPNOTSUPP between these two hunks to avoid missing the domain_unpause() It would probably be better to pull the check forwards to before the domain_pause(). > } else > ad->monitor.mov_to_msr_extended = 0; > > - domain_pause(d); > ad->monitor.mov_to_msr_enabled = !status; > domain_unpause(d); > break; > @@ -196,9 +199,8 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > The other codepaths look fine. ~Andrew
On 01/28/2016 04:10 PM, Andrew Cooper wrote: > On 28/01/16 13:52, Razvan Cojocaru wrote: >> This patch pauses the domain for all writes through the 'ad' >> pointer in monitor_domctl(), defers a domain_unpause() call until >> after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG >> case, and makes sure that the domain is paused for both vm_event >> enable and disable cases in vm_event_domctl(). >> Thanks go to Andrew Cooper for his review and suggestions. >> >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > > Would you mind annotating each of the checks for d != current->domain > with /* no domain_pause(). */, which is our normal practice. Of course, no problem. > I think it might be better to move the current check for > XEN_DOMCTL_monitor_op into monitor_domctl() itself, to have all checks > together in one place. I'll move it. >> @@ -144,6 +146,8 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) >> if ( rc ) >> return rc; >> >> + domain_pause(d); >> + >> if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && >> mop->u.mov_to_msr.extended_capture ) >> { >> @@ -154,7 +158,6 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > > You will need to rework the return -EOPNOTSUPP between these two hunks > to avoid missing the domain_unpause() > > It would probably be better to pull the check forwards to before the > domain_pause(). Right, sorry I've missed that. Thanks! >> } else >> ad->monitor.mov_to_msr_extended = 0; >> >> - domain_pause(d); >> ad->monitor.mov_to_msr_enabled = !status; >> domain_unpause(d); >> break; >> @@ -196,9 +199,8 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) >> > > The other codepaths look fine. Thanks for the review, Razvan
On 01/28/2016 04:10 PM, Andrew Cooper wrote: > On 28/01/16 13:52, Razvan Cojocaru wrote: >> This patch pauses the domain for all writes through the 'ad' >> pointer in monitor_domctl(), defers a domain_unpause() call until >> after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG >> case, and makes sure that the domain is paused for both vm_event >> enable and disable cases in vm_event_domctl(). >> Thanks go to Andrew Cooper for his review and suggestions. >> >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > > Would you mind annotating each of the checks for d != current->domain > with /* no domain_pause(). */, which is our normal practice. Nice, doing that allowed me to find and modify this code: 1137 #ifdef CONFIG_HAS_MEM_ACCESS 1138 case XEN_DOMCTL_set_access_required: 1139 if ( unlikely(current->domain == d) ) /* no domain_pause() */ 1140 ret = -EPERM; 1141 else 1142 { 1143 domain_pause(d); 1144 p2m_get_hostp2m(d)->access_required = 1145 op->u.access_required.access_required; 1146 domain_unpause(d); 1147 } 1148 break; 1149 #endif (there was no domain_pause(d) / domain_unpause(d)) before. Thanks, Razvan
On Thu, Jan 28, 2016 at 6:52 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > This patch pauses the domain for all writes through the 'ad' > pointer in monitor_domctl(), defers a domain_unpause() call until > after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG > case, and makes sure that the domain is paused for both vm_event > enable and disable cases in vm_event_domctl(). > Thanks go to Andrew Cooper for his review and suggestions. > For vm_event_enable the domain is already paused by libxc before the domctl is issued. I don't see a problem in doing another pause in Xen, but given we have XSA-99, just doing this pause in Xen would not be enough. So is it really necessary/fixes anything? Tamas
On 01/28/2016 10:09 PM, Tamas K Lengyel wrote: > On Thu, Jan 28, 2016 at 6:52 AM, Razvan Cojocaru > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: > > This patch pauses the domain for all writes through the 'ad' > pointer in monitor_domctl(), defers a domain_unpause() call until > after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG > case, and makes sure that the domain is paused for both vm_event > enable and disable cases in vm_event_domctl(). > Thanks go to Andrew Cooper for his review and suggestions. > > > For vm_event_enable the domain is already paused by libxc before the > domctl is issued. I don't see a problem in doing another pause in Xen, > but given we have XSA-99, just doing this pause in Xen would not be > enough. So is it really necessary/fixes anything? This isn't about XSA-99, the problem here is related to my previous patch "x86 vm_event: reset monitor in vm_event_cleanup_domain()". While that improves matters and greatly reduces the chances of crashes due to hvm_msr_write_intercept() or hvm_set_crX() dereferencing a NULL v->arch.vm_event that's assumed to be OK, when the corresponding v->domain->arch.monitor is non-zero, the foolproof way is to make sure that functions such as vm_event_cleanup_domain() are always being called only while the domain has been paused. So there should be a domain_pause() call somewhere on the call path before that. In fact, any writes to domain-scope variables that can affect the way a guest runs (while the guest is running) should ideally be done with the domain paused, hence the rest of the changes in the patch. Thanks, Razvan
On Thu, Jan 28, 2016 at 2:05 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 01/28/2016 10:09 PM, Tamas K Lengyel wrote: > > On Thu, Jan 28, 2016 at 6:52 AM, Razvan Cojocaru > > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: > > > > This patch pauses the domain for all writes through the 'ad' > > pointer in monitor_domctl(), defers a domain_unpause() call until > > after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG > > case, and makes sure that the domain is paused for both vm_event > > enable and disable cases in vm_event_domctl(). > > Thanks go to Andrew Cooper for his review and suggestions. > > > > > > For vm_event_enable the domain is already paused by libxc before the > > domctl is issued. I don't see a problem in doing another pause in Xen, > > but given we have XSA-99, just doing this pause in Xen would not be > > enough. So is it really necessary/fixes anything? > > This isn't about XSA-99, the problem here is related to my previous > patch "x86 vm_event: reset monitor in vm_event_cleanup_domain()". While > that improves matters and greatly reduces the chances of crashes due to > hvm_msr_write_intercept() or hvm_set_crX() dereferencing a NULL > v->arch.vm_event that's assumed to be OK, when the corresponding > v->domain->arch.monitor is non-zero, the foolproof way is to make sure > that functions such as vm_event_cleanup_domain() are always being called > only while the domain has been paused. So there should be a > domain_pause() call somewhere on the call path before that. > Sure, but that's the disable case. I was only wondering about the enable case where the domain is already paused. Tamas
On 01/28/2016 11:47 PM, Tamas K Lengyel wrote: > > > On Thu, Jan 28, 2016 at 2:05 PM, Razvan Cojocaru > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: > > On 01/28/2016 10:09 PM, Tamas K Lengyel wrote: > > On Thu, Jan 28, 2016 at 6:52 AM, Razvan Cojocaru > > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com> > <mailto:rcojocaru@bitdefender.com > <mailto:rcojocaru@bitdefender.com>>> wrote: > > > > This patch pauses the domain for all writes through the 'ad' > > pointer in monitor_domctl(), defers a domain_unpause() call until > > after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG > > case, and makes sure that the domain is paused for both vm_event > > enable and disable cases in vm_event_domctl(). > > Thanks go to Andrew Cooper for his review and suggestions. > > > > > > For vm_event_enable the domain is already paused by libxc before the > > domctl is issued. I don't see a problem in doing another pause in Xen, > > but given we have XSA-99, just doing this pause in Xen would not be > > enough. So is it really necessary/fixes anything? > > This isn't about XSA-99, the problem here is related to my previous > patch "x86 vm_event: reset monitor in vm_event_cleanup_domain()". While > that improves matters and greatly reduces the chances of crashes due to > hvm_msr_write_intercept() or hvm_set_crX() dereferencing a NULL > v->arch.vm_event that's assumed to be OK, when the corresponding > v->domain->arch.monitor is non-zero, the foolproof way is to make sure > that functions such as vm_event_cleanup_domain() are always being called > only while the domain has been paused. So there should be a > domain_pause() call somewhere on the call path before that. > > > Sure, but that's the disable case. I was only wondering about the enable > case where the domain is already paused. Yes, for the disable case it is functionally redundant. I just thought it would be consistent to use domain_pause() throughout and didn't think it would hurt anything. But I have nothing against removing the domain_pause() for the enable case (perhaps replacing it with a comment that libxc already pauses externally), unless Andrew has any objections. Thanks, Razvan
On 01/29/2016 08:52 AM, Razvan Cojocaru wrote: > On 01/28/2016 11:47 PM, Tamas K Lengyel wrote: >> >> >> On Thu, Jan 28, 2016 at 2:05 PM, Razvan Cojocaru >> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: >> >> On 01/28/2016 10:09 PM, Tamas K Lengyel wrote: >> > On Thu, Jan 28, 2016 at 6:52 AM, Razvan Cojocaru >> > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com> >> <mailto:rcojocaru@bitdefender.com >> <mailto:rcojocaru@bitdefender.com>>> wrote: >> > >> > This patch pauses the domain for all writes through the 'ad' >> > pointer in monitor_domctl(), defers a domain_unpause() call until >> > after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG >> > case, and makes sure that the domain is paused for both vm_event >> > enable and disable cases in vm_event_domctl(). >> > Thanks go to Andrew Cooper for his review and suggestions. >> > >> > >> > For vm_event_enable the domain is already paused by libxc before the >> > domctl is issued. I don't see a problem in doing another pause in Xen, >> > but given we have XSA-99, just doing this pause in Xen would not be >> > enough. So is it really necessary/fixes anything? >> >> This isn't about XSA-99, the problem here is related to my previous >> patch "x86 vm_event: reset monitor in vm_event_cleanup_domain()". While >> that improves matters and greatly reduces the chances of crashes due to >> hvm_msr_write_intercept() or hvm_set_crX() dereferencing a NULL >> v->arch.vm_event that's assumed to be OK, when the corresponding >> v->domain->arch.monitor is non-zero, the foolproof way is to make sure >> that functions such as vm_event_cleanup_domain() are always being called >> only while the domain has been paused. So there should be a >> domain_pause() call somewhere on the call path before that. >> >> >> Sure, but that's the disable case. I was only wondering about the enable >> case where the domain is already paused. > > Yes, for the disable case it is functionally redundant. Sorry, I obviously meant "for the enable case".
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 7611f7b..bb1920a 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -80,7 +80,9 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) return 0; case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: - d->arch.mem_access_emulate_each_rep = !!mop->event; + domain_pause(d); + ad->mem_access_emulate_each_rep = !!mop->event; + domain_unpause(d); return 0; } @@ -109,6 +111,8 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) if ( rc ) return rc; + domain_pause(d); + if ( mop->u.mov_to_cr.sync ) ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask; else @@ -119,20 +123,18 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) else ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask; - domain_pause(d); - if ( !status ) ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask; else ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask; - domain_unpause(d); - if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 ) /* Latches new CR3 mask through CR0 code */ for_each_vcpu ( d, v ) hvm_update_guest_cr(v, 0); + domain_unpause(d); + break; } @@ -144,6 +146,8 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) if ( rc ) return rc; + domain_pause(d); + if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && mop->u.mov_to_msr.extended_capture ) { @@ -154,7 +158,6 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) } else ad->monitor.mov_to_msr_extended = 0; - domain_pause(d); ad->monitor.mov_to_msr_enabled = !status; domain_unpause(d); break; @@ -196,9 +199,8 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) if ( rc ) return rc; - ad->monitor.guest_request_sync = mop->u.guest_request.sync; - domain_pause(d); + ad->monitor.guest_request_sync = mop->u.guest_request.sync; ad->monitor.guest_request_enabled = !status; domain_unpause(d); break; diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index 28a7add..9d18c8d 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -624,15 +624,21 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, if ( p2m->pod.entry_count ) break; + domain_pause(d); rc = vm_event_enable(d, vec, ved, _VPF_mem_paging, HVM_PARAM_PAGING_RING_PFN, mem_paging_notification); + domain_unpause(d); } break; case XEN_VM_EVENT_DISABLE: if ( ved->ring_page ) + { + domain_pause(d); rc = vm_event_disable(d, ved); + domain_unpause(d); + } break; case XEN_VM_EVENT_RESUME: @@ -658,14 +664,20 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, switch( vec->op ) { case XEN_VM_EVENT_ENABLE: + domain_pause(d); rc = vm_event_enable(d, vec, ved, _VPF_mem_access, HVM_PARAM_MONITOR_RING_PFN, monitor_notification); + domain_unpause(d); break; case XEN_VM_EVENT_DISABLE: if ( ved->ring_page ) + { + domain_pause(d); rc = vm_event_disable(d, ved); + domain_unpause(d); + } break; case XEN_VM_EVENT_RESUME: @@ -701,14 +713,20 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, if ( !hap_enabled(d) ) break; + domain_pause(d); rc = vm_event_enable(d, vec, ved, _VPF_mem_sharing, HVM_PARAM_SHARING_RING_PFN, mem_sharing_notification); + domain_unpause(d); break; case XEN_VM_EVENT_DISABLE: if ( ved->ring_page ) + { + domain_pause(d); rc = vm_event_disable(d, ved); + domain_unpause(d); + } break; case XEN_VM_EVENT_RESUME:
This patch pauses the domain for all writes through the 'ad' pointer in monitor_domctl(), defers a domain_unpause() call until after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG case, and makes sure that the domain is paused for both vm_event enable and disable cases in vm_event_domctl(). Thanks go to Andrew Cooper for his review and suggestions. Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> --- xen/arch/x86/monitor.c | 18 ++++++++++-------- xen/common/vm_event.c | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-)