[Xen-devel] [PATCH v4 2/3] x86/hvm: introduce functions for HVMOP_get/set_param allowance checks

[Paul Durrant <paul.durrant@xxxxxxxxxx>]

Some parameters can only (validly) be set once. Some should not be set
by a guest for its own domain, and others must not be set since they
require the domain to be paused. Consolidate these checks, along with
the XSM check, in a new hvm_allow_set_param() function for clarity.

Also, introduce hvm_allow_get_param() for similar reasons.

NOTE: This patch leaves the guest accessible parameter checks as
      black-lists and does not make any changes to which parameters
      can be accessed. A subsequent patch will tighten up these
      checks by changing from black-lists to white-lists.

Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
Cc: Keir Fraser <keir@xxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
 xen/arch/x86/hvm/hvm.c |  153 ++++++++++++++++++++++++++++--------------------
 1 file changed, 90 insertions(+), 63 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c246b45..03543dd 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5638,6 +5638,61 @@ static int hvmop_set_evtchn_upcall_vector(
     return 0;
 }
 
+static int hvm_allow_set_param(struct domain *d,
+                               const struct xen_hvm_param *a)
+{
+    uint64_t value = d->arch.hvm_domain.params[a->index];
+    int rc;
+
+    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
+    if ( rc )
+        return rc;
+
+    switch ( a->index )
+    {
+    /*
+     * The following parameters must not be set by the guest
+     * since the domain may need to be paused.
+     */
+    case HVM_PARAM_IDENT_PT:
+    case HVM_PARAM_DM_DOMAIN:
+    case HVM_PARAM_ACPI_S_STATE:
+    /* The following parameters should not be set by the guest. */
+    case HVM_PARAM_VIRIDIAN:
+    case HVM_PARAM_MEMORY_EVENT_CR0:
+    case HVM_PARAM_MEMORY_EVENT_CR3:
+    case HVM_PARAM_MEMORY_EVENT_CR4:
+    case HVM_PARAM_MEMORY_EVENT_INT3:
+    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
+    case HVM_PARAM_MEMORY_EVENT_MSR:
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+        if ( d == current->domain )
+            rc = -EPERM;
+        break;
+    default:
+        break;
+    }
+
+    if ( rc )
+        return rc;
+
+    switch ( a->index )
+    {
+    /* The following parameters should only be changed once. */
+    case HVM_PARAM_VIRIDIAN:
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+        if ( value != 0 && a->value != value )
+            rc = -EEXIST;
+        break;
+    default:
+        break;
+    }
+
+    return rc;
+}
+
 static int hvmop_set_param(
     XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
 {
@@ -5662,7 +5717,7 @@ static int hvmop_set_param(
          (is_pvh_domain(d) && (a.index != HVM_PARAM_CALLBACK_IRQ)) )
         goto out;
 
-    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
+    rc = hvm_allow_set_param(d, &a);
     if ( rc )
         goto out;
 
@@ -5677,31 +5732,11 @@ static int hvmop_set_param(
             rc = -EINVAL;
         break;
     case HVM_PARAM_VIRIDIAN:
-        /* This should only ever be set once by the tools and read by the 
guest. */
-        rc = -EPERM;
-        if ( d == curr_d )
-            break;
-
-        if ( a.value != d->arch.hvm_domain.params[a.index] )
-        {
-            rc = -EEXIST;
-            if ( d->arch.hvm_domain.params[a.index] != 0 )
-                break;
-
+        if ( (a.value & ~HVMPV_feature_mask) ||
+             !(a.value & HVMPV_base_freq) )
             rc = -EINVAL;
-            if ( (a.value & ~HVMPV_feature_mask) ||
-                 !(a.value & HVMPV_base_freq) )
-                break;
-        }
-
-        rc = 0;
         break;
     case HVM_PARAM_IDENT_PT:
-        /* Not reflexive, as we must domain_pause(). */
-        rc = -EPERM;
-        if ( d == curr_d )
-            break;
-
         rc = -EINVAL;
         if ( d->arch.hvm_domain.params[a.index] != 0 )
             break;
@@ -5729,22 +5764,12 @@ static int hvmop_set_param(
         domctl_lock_release();
         break;
     case HVM_PARAM_DM_DOMAIN:
-        /* Not reflexive, as we may need to domain_pause(). */
-        rc = -EPERM;
-        if ( d == curr_d )
-            break;
-
         if ( a.value == DOMID_SELF )
             a.value = curr_d->domain_id;
 
         rc = hvm_set_dm_domain(d, a.value);
         break;
     case HVM_PARAM_ACPI_S_STATE:
-        /* Not reflexive, as we must domain_pause(). */
-        rc = -EPERM;
-        if ( d == curr_d )
-            break;
-
         rc = 0;
         if ( a.value == 3 )
             hvm_s3_suspend(d);
@@ -5757,20 +5782,9 @@ static int hvmop_set_param(
     case HVM_PARAM_ACPI_IOPORTS_LOCATION:
         rc = pmtimer_change_ioport(d, a.value);
         break;
-    case HVM_PARAM_MEMORY_EVENT_CR0:
-    case HVM_PARAM_MEMORY_EVENT_CR3:
-    case HVM_PARAM_MEMORY_EVENT_CR4:
-        if ( d == curr_d )
-            rc = -EPERM;
-        break;
     case HVM_PARAM_MEMORY_EVENT_INT3:
     case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
     case HVM_PARAM_MEMORY_EVENT_MSR:
-        if ( d == curr_d )
-        {
-            rc = -EPERM;
-            break;
-        }
         if ( a.value & HVMPME_onchangeonly )
             rc = -EINVAL;
         break;
@@ -5804,22 +5818,12 @@ static int hvmop_set_param(
             rc = -EINVAL;
         break;
     case HVM_PARAM_IOREQ_SERVER_PFN:
-        if ( d == curr_d )
-        {
-            rc = -EPERM;
-            break;
-        }
         d->arch.hvm_domain.ioreq_gmfn.base = a.value;
         break;
     case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
     {
         unsigned int i;
 
-        if ( d == curr_d )
-        {
-            rc = -EPERM;
-            break;
-        }
         if ( a.value == 0 ||
              a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
         {
@@ -5859,10 +5863,40 @@ static int hvmop_set_param(
     return rc;
 }
 
+static int hvm_allow_get_param(struct domain *d,
+                               const struct xen_hvm_param *a)
+{
+    int rc;
+
+    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
+    if ( rc )
+        return rc;
+
+    switch ( a->index )
+    {
+    /*
+     * The following parameters must not be read by the guest
+     * since the domain may need to be paused.
+     */
+    case HVM_PARAM_IOREQ_PFN:
+    case HVM_PARAM_BUFIOREQ_PFN:
+    case HVM_PARAM_BUFIOREQ_EVTCHN:
+    /* The following parameters should not be read by the guest. */
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+        if ( d == current->domain )
+            rc = -EPERM;
+        break;
+    default:
+        break;
+    }
+
+    return rc;
+}
+
 static int hvmop_get_param(
     XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
 {
-    struct domain *curr_d = current->domain;
     struct xen_hvm_param a;
     struct domain *d;
     int rc;
@@ -5882,7 +5916,7 @@ static int hvmop_get_param(
          (is_pvh_domain(d) && (a.index != HVM_PARAM_CALLBACK_IRQ)) )
         goto out;
 
-    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
+    rc = hvm_allow_get_param(d, &a);
     if ( rc )
         goto out;
 
@@ -5891,13 +5925,6 @@ static int hvmop_get_param(
     case HVM_PARAM_ACPI_S_STATE:
         a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0;
         break;
-    case HVM_PARAM_IOREQ_SERVER_PFN:
-    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
-        if ( d == curr_d )
-        {
-            rc = -EPERM;
-            goto out;
-        }
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
-- 
1.7.10.4


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