[PATCH] vm_event: make sure the domain is paused in key domctls

[PATCH] vm_event: make sure the domain is paused in key domctls

[Razvan Cojocaru]

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(-)

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:
-- 
1.9.1

Re: [PATCH] vm_event: make sure the domain is paused in key domctls

[Andrew Cooper]

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

Re: [PATCH] vm_event: make sure the domain is paused in key domctls

[Razvan Cojocaru]

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

Re: [PATCH] vm_event: make sure the domain is paused in key domctls

[Razvan Cojocaru]

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

Re: [PATCH] vm_event: make sure the domain is paused in key domctls

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 725 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 1167 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: make sure the domain is paused in key domctls

[Razvan Cojocaru]

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

Re: [PATCH] vm_event: make sure the domain is paused in key domctls

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1704 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 2402 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: make sure the domain is paused in key domctls

[Razvan Cojocaru]

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

Re: [PATCH] vm_event: make sure the domain is paused in key domctls

[Razvan Cojocaru]

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".