[PATCH V2] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL

[PATCH V2] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL

[Razvan Cojocaru]

It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear())
to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates
vcpu->arch.vm_event) has been called, so return an error from the
XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
Changes since V1:
 - Fixed the if() condition.
 - Introduced an rc return variable to simplify the code.
---
 xen/include/asm-x86/monitor.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 0954b59..a66760d 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -32,19 +32,29 @@
 static inline
 int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
 {
+    int rc = 0;
+
     switch ( mop->op )
     {
     case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
         domain_pause(d);
-        d->arch.mem_access_emulate_each_rep = !!mop->event;
+        /*
+         * Enabling mem_access_emulate_each_rep without a vm_event subscriber
+         * is meaningless.
+         */
+        if ( d->vcpu && d->vcpu[0]->arch.vm_event )
+            d->arch.mem_access_emulate_each_rep = !!mop->event;
+        else
+            rc = -EINVAL;
+
         domain_unpause(d);
         break;
 
     default:
-        return -EOPNOTSUPP;
+        rc = -EOPNOTSUPP;
     }
 
-    return 0;
+    return rc;
 }
 
 int arch_monitor_domctl_event(struct domain *d,
-- 
1.9.1


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

Re: [PATCH V2] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL

[Andrew Cooper]

On 08/04/16 13:35, Razvan Cojocaru wrote:
> It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear())
> to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates
> vcpu->arch.vm_event) has been called, so return an error from the
> XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>
> ---
> Changes since V1:
>  - Fixed the if() condition.
>  - Introduced an rc return variable to simplify the code.
> ---
>  xen/include/asm-x86/monitor.h | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 0954b59..a66760d 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -32,19 +32,29 @@
>  static inline
>  int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>  {
> +    int rc = 0;
> +
>      switch ( mop->op )
>      {
>      case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
>          domain_pause(d);
> -        d->arch.mem_access_emulate_each_rep = !!mop->event;
> +        /*
> +         * Enabling mem_access_emulate_each_rep without a vm_event subscriber
> +         * is meaningless.
> +         */
> +        if ( d->vcpu && d->vcpu[0]->arch.vm_event )

Sorry - I forgot to mention this before, but this check is slightly
buggy.  You want

if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event )

d->max_vcpus being non-zero guarentees that d->vcpu has been allocated,
but you still need to check that d->vcpu[0] has been allocated before
following the pointer.

With that fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com>

> +            d->arch.mem_access_emulate_each_rep = !!mop->event;
> +        else
> +            rc = -EINVAL;
> +
>          domain_unpause(d);
>          break;
>  
>      default:
> -        return -EOPNOTSUPP;
> +        rc = -EOPNOTSUPP;
>      }
>  
> -    return 0;
> +    return rc;
>  }
>  
>  int arch_monitor_domctl_event(struct domain *d,


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