[PATCH v2 0/3] x86/hvm: HVMOP_get/set_param improvements

[PATCH v2 0/3] x86/hvm: HVMOP_get/set_param improvements

[Paul Durrant]

The following 3 patches re-structure the code implementing HVMOP_set_param
and HVMOP_get_param.

Patch #1 gives each operation its own function

Patch #2 splits out checks for getting/setting non-reflexive params and
setting params with change-once semantics, as well as the XSM check into
separate functions

Patch #3 swaps from black-lists to white-lists for guest accessible
params

v2:
 - Fix style issues during code movement in patch #2.
 - End up with white-lists rather than black-lists for guest accessible
   params.
 - Re-work comments on patch #2 slightly.

[PATCH 0/2] x86/hvm: HVMOP_get/set_param improvements

[Paul Durrant]

The following 3 patches re-structure the code implementing HVMOP_set_param
and HVMOP_get_param.

Patch #1 gives each operation its own function

Patch #2 splits out checks for getting/setting non-reflexive params and
setting params with change-once semantics, as well as the XSM check into
separate functions

Patch #3 adds all remaining ioreq server params to non-reflexive lists

Re: [PATCH 0/2] x86/hvm: HVMOP_get/set_param improvements

[Paul Durrant]

Sorry, this was an artefact of the previous set... Please ignore.

  Paul

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 01 May 2015 15:06
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant
> Subject: [PATCH 0/2] x86/hvm: HVMOP_get/set_param improvements
> 
> The following 3 patches re-structure the code implementing
> HVMOP_set_param
> and HVMOP_get_param.
> 
> Patch #1 gives each operation its own function
> 
> Patch #2 splits out checks for getting/setting non-reflexive params and
> setting params with change-once semantics, as well as the XSM check into
> separate functions
> 
> Patch #3 adds all remaining ioreq server params to non-reflexive lists

[PATCH v2 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions

[Paul Durrant]

The level of switch nesting in those ops is getting unreadable. Giving
them their own functions does introduce some code duplication in the
the pre-op checks but the overall result is easier to follow.

This patch is code movement (including style fixes). There is no
functional change.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |  561 ++++++++++++++++++++++++++----------------------
 1 file changed, 299 insertions(+), 262 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index bfde380..ab71202 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5638,6 +5638,299 @@ static int hvmop_set_evtchn_upcall_vector(
     return 0;
 }
 
+static int hvmop_set_param(
+    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
+{
+    struct domain *curr_d = current->domain;
+    struct xen_hvm_param a;
+    struct domain *d;
+    struct vcpu *v;
+    int rc = 0;
+
+    if ( copy_from_guest(&a, arg, 1) )
+        return -EFAULT;
+
+    if ( a.index >= HVM_NR_PARAMS )
+        return -EINVAL;
+
+    d = rcu_lock_domain_by_any_id(a.domid);
+    if ( d == NULL )
+        return -ESRCH;
+
+    rc = -EINVAL;
+    if ( !has_hvm_container_domain(d) )
+        goto out;
+
+    if ( is_pvh_domain(d)
+         && (a.index != HVM_PARAM_CALLBACK_IRQ) )
+        goto out;
+
+    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
+    if ( rc )
+        goto out;
+
+    switch ( a.index )
+    {
+    case HVM_PARAM_CALLBACK_IRQ:
+        hvm_set_callback_via(d, a.value);
+        hvm_latch_shinfo_size(d);
+        break;
+    case HVM_PARAM_TIMER_MODE:
+        if ( a.value > HVMPTM_one_missed_tick_pending )
+            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;
+
+            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;
+
+        rc = 0;
+        if ( !paging_mode_hap(d) )
+            break;
+
+        /*
+         * Update GUEST_CR3 in each VMCS to point at identity map.
+         * All foreign updates to guest state must synchronise on
+         * the domctl_lock.
+         */
+        rc = -ERESTART;
+        if ( !domctl_lock_acquire() )
+            break;
+
+        rc = 0;
+        domain_pause(d);
+        d->arch.hvm_domain.params[a.index] = a.value;
+        for_each_vcpu ( d, v )
+            paging_update_cr3(v);
+        domain_unpause(d);
+
+        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);
+        else if ( a.value == 0 )
+            hvm_s3_resume(d);
+        else
+            rc = -EINVAL;
+
+        break;
+    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;
+    case HVM_PARAM_NESTEDHVM:
+        rc = xsm_hvm_param_nested(XSM_PRIV, d);
+        if ( rc )
+            break;
+        if ( a.value > 1 )
+            rc = -EINVAL;
+        /* Remove the check below once we have
+         * shadow-on-shadow.
+         */
+        if ( cpu_has_svm && !paging_mode_hap(d) && a.value )
+            rc = -EINVAL;
+        /* Set up NHVM state for any vcpus that are already up */
+        if ( a.value &&
+             !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] )
+            for_each_vcpu(d, v)
+                if ( rc == 0 )
+                    rc = nestedhvm_vcpu_initialise(v);
+        if ( !a.value || rc )
+            for_each_vcpu(d, v)
+                nestedhvm_vcpu_destroy(v);
+        break;
+    case HVM_PARAM_BUFIOREQ_EVTCHN:
+        rc = -EINVAL;
+        break;
+    case HVM_PARAM_TRIPLE_FAULT_REASON:
+        if ( a.value > SHUTDOWN_MAX )
+            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 )
+        {
+            rc = -EINVAL;
+            break;
+        }
+        for ( i = 0; i < a.value; i++ )
+            set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
+
+        break;
+    }
+    }
+
+    if ( rc != 0 )
+        goto out;
+
+    d->arch.hvm_domain.params[a.index] = a.value;
+
+    switch( a.index )
+    {
+    case HVM_PARAM_MEMORY_EVENT_INT3:
+    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
+        domain_pause(d);
+        domain_unpause(d); /* Causes guest to latch new status */
+        break;
+    case HVM_PARAM_MEMORY_EVENT_CR3:
+        for_each_vcpu ( d, v )
+            hvm_funcs.update_guest_cr(v, 0); /* Latches new CR3 mask through CR0 code */
+        break;
+    }
+
+    HVM_DBG_LOG(DBG_LEVEL_HCALL, "set param %u = %"PRIx64,
+                a.index, a.value);
+
+ out:
+    rcu_unlock_domain(d);
+    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 = 0;
+
+    if ( copy_from_guest(&a, arg, 1) )
+        return -EFAULT;
+
+    if ( a.index >= HVM_NR_PARAMS )
+        return -EINVAL;
+
+    d = rcu_lock_domain_by_any_id(a.domid);
+    if ( d == NULL )
+        return -ESRCH;
+
+    rc = -EINVAL;
+    if ( !has_hvm_container_domain(d) )
+        goto out;
+
+    if ( is_pvh_domain(d)
+         && (a.index != HVM_PARAM_CALLBACK_IRQ) )
+        goto out;
+
+    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
+    if ( rc )
+        goto out;
+
+    switch ( a.index )
+    {
+    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:
+    {
+        domid_t domid;
+
+        /* May need to create server */
+        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
+        rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL);
+        if ( rc != 0 && rc != -EEXIST )
+            goto out;
+        /*FALLTHRU*/
+    }
+    default:
+        a.value = d->arch.hvm_domain.params[a.index];
+        break;
+    }
+
+    rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+
+    HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64,
+                a.index, a.value);
+
+ out:
+    rcu_unlock_domain(d);
+    return rc;
+}
+
 /*
  * Note that this value is effectively part of the ABI, even if we don't need
  * to make it a formal part of it: A guest suspended for migration in the
@@ -5649,7 +5942,6 @@ static int hvmop_set_evtchn_upcall_vector(
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
 {
-    struct domain *curr_d = current->domain;
     unsigned long start_iter, mask;
     long rc = 0;
 
@@ -5703,269 +5995,14 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     
     case HVMOP_set_param:
-    case HVMOP_get_param:
-    {
-        struct xen_hvm_param a;
-        struct domain *d;
-        struct vcpu *v;
-
-        if ( copy_from_guest(&a, arg, 1) )
-            return -EFAULT;
-
-        if ( a.index >= HVM_NR_PARAMS )
-            return -EINVAL;
-
-        d = rcu_lock_domain_by_any_id(a.domid);
-        if ( d == NULL )
-            return -ESRCH;
-
-        rc = -EINVAL;
-        if ( !has_hvm_container_domain(d) )
-            goto param_fail;
-
-        if ( is_pvh_domain(d)
-             && (a.index != HVM_PARAM_CALLBACK_IRQ) )
-            goto param_fail;
-
-        rc = xsm_hvm_param(XSM_TARGET, d, op);
-        if ( rc )
-            goto param_fail;
-
-        if ( op == HVMOP_set_param )
-        {
-            rc = 0;
-
-            switch ( a.index )
-            {
-            case HVM_PARAM_CALLBACK_IRQ:
-                hvm_set_callback_via(d, a.value);
-                hvm_latch_shinfo_size(d);
-                break;
-            case HVM_PARAM_TIMER_MODE:
-                if ( a.value > HVMPTM_one_missed_tick_pending )
-                    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 ( curr_d == d )
-                    break;
-
-                if ( a.value != d->arch.hvm_domain.params[a.index] )
-                {
-                    rc = -EEXIST;
-                    if ( d->arch.hvm_domain.params[a.index] != 0 )
-                        break;
-
-                    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 ( curr_d == d )
-                    break;
-
-                rc = -EINVAL;
-                if ( d->arch.hvm_domain.params[a.index] != 0 )
-                    break;
-
-                rc = 0;
-                if ( !paging_mode_hap(d) )
-                    break;
-
-                /*
-                 * Update GUEST_CR3 in each VMCS to point at identity map.
-                 * All foreign updates to guest state must synchronise on
-                 * the domctl_lock.
-                 */
-                rc = -ERESTART;
-                if ( !domctl_lock_acquire() )
-                    break;
-
-                rc = 0;
-                domain_pause(d);
-                d->arch.hvm_domain.params[a.index] = a.value;
-                for_each_vcpu ( d, v )
-                    paging_update_cr3(v);
-                domain_unpause(d);
-
-                domctl_lock_release();
-                break;
-            case HVM_PARAM_DM_DOMAIN:
-                /* Not reflexive, as we may need to domain_pause(). */
-                rc = -EPERM;
-                if ( curr_d == 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 ( curr_d == d )
-                    break;
-
-                rc = 0;
-                if ( a.value == 3 )
-                    hvm_s3_suspend(d);
-                else if ( a.value == 0 )
-                    hvm_s3_resume(d);
-                else
-                    rc = -EINVAL;
-
-                break;
-            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 == current->domain )
-                    rc = -EPERM;
-                break;
-            case HVM_PARAM_MEMORY_EVENT_INT3:
-            case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
-            case HVM_PARAM_MEMORY_EVENT_MSR:
-                if ( d == current->domain )
-                {
-                    rc = -EPERM;
-                    break;
-                }
-                if ( a.value & HVMPME_onchangeonly )
-                    rc = -EINVAL;
-                break;
-            case HVM_PARAM_NESTEDHVM:
-                rc = xsm_hvm_param_nested(XSM_PRIV, d);
-                if ( rc )
-                    break;
-                if ( a.value > 1 )
-                    rc = -EINVAL;
-                /* Remove the check below once we have
-                 * shadow-on-shadow.
-                 */
-                if ( cpu_has_svm && !paging_mode_hap(d) && a.value )
-                    rc = -EINVAL;
-                /* Set up NHVM state for any vcpus that are already up */
-                if ( a.value &&
-                     !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] )
-                    for_each_vcpu(d, v)
-                        if ( rc == 0 )
-                            rc = nestedhvm_vcpu_initialise(v);
-                if ( !a.value || rc )
-                    for_each_vcpu(d, v)
-                        nestedhvm_vcpu_destroy(v);
-                break;
-            case HVM_PARAM_BUFIOREQ_EVTCHN:
-                rc = -EINVAL;
-                break;
-            case HVM_PARAM_TRIPLE_FAULT_REASON:
-                if ( a.value > SHUTDOWN_MAX )
-                    rc = -EINVAL;
-                break;
-            case HVM_PARAM_IOREQ_SERVER_PFN:
-                if ( d == current->domain )
-                {
-                    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 == current->domain )
-                {
-                    rc = -EPERM;
-                    break;
-                }
-                if ( a.value == 0 ||
-                     a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
-                {
-                    rc = -EINVAL;
-                    break;
-                }
-                for ( i = 0; i < a.value; i++ )
-                    set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
-
-                break;
-            }
-            }
-
-            if ( rc == 0 ) 
-            {
-                d->arch.hvm_domain.params[a.index] = a.value;
-
-                switch( a.index )
-                {
-                case HVM_PARAM_MEMORY_EVENT_INT3:
-                case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
-                {
-                    domain_pause(d);
-                    domain_unpause(d); /* Causes guest to latch new status */
-                    break;
-                }
-                case HVM_PARAM_MEMORY_EVENT_CR3:
-                {
-                    for_each_vcpu ( d, v )
-                        hvm_funcs.update_guest_cr(v, 0); /* Latches new CR3 mask through CR0 code */
-                    break;
-                }
-                }
-
-            }
-
-        }
-        else
-        {
-            switch ( a.index )
-            {
-            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 == current->domain )
-                {
-                    rc = -EPERM;
-                    break;
-                }
-            case HVM_PARAM_IOREQ_PFN:
-            case HVM_PARAM_BUFIOREQ_PFN:
-            case HVM_PARAM_BUFIOREQ_EVTCHN: {
-                domid_t domid;
-                
-                /* May need to create server */
-                domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
-                rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL);
-                if ( rc != 0 && rc != -EEXIST )
-                    goto param_fail;
-                /*FALLTHRU*/
-            }
-            default:
-                a.value = d->arch.hvm_domain.params[a.index];
-                break;
-            }
-            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
-        }
-
-        HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64,
-                    op == HVMOP_set_param ? "set" : "get",
-                    a.index, a.value);
+        rc = hvmop_set_param(
+            guest_handle_cast(arg, xen_hvm_param_t));
+        break;
 
-    param_fail:
-        rcu_unlock_domain(d);
+    case HVMOP_get_param:
+        rc = hvmop_get_param(
+            guest_handle_cast(arg, xen_hvm_param_t));
         break;
-    }
 
     case HVMOP_set_pci_intx_level:
         rc = hvmop_set_pci_intx_level(
-- 
1.7.10.4

Re: [PATCH v2 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions

[Jan Beulich]

>>> Paul Durrant <paul.durrant@citrix.com> 05/01/15 4:05 PM >>>
>--- a/xen/arch/x86/hvm/hvm.c
>+++ b/xen/arch/x86/hvm/hvm.c
>@@ -5638,6 +5638,299 @@ static int hvmop_set_evtchn_upcall_vector(
>return 0;
>}
 >
>+static int hvmop_set_param(
>+    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
>+{
>+    struct domain *curr_d = current->domain;
>+    struct xen_hvm_param a;
>+    struct domain *d;
>+    struct vcpu *v;
>+    int rc = 0;

Iirc Andrew indicated that Coverity would complain about dead initializers like
this.

>+    if ( copy_from_guest(&a, arg, 1) )
>+        return -EFAULT;
>+
>+    if ( a.index >= HVM_NR_PARAMS )
>+        return -EINVAL;
>+
>+    d = rcu_lock_domain_by_any_id(a.domid);
>+    if ( d == NULL )
>+        return -ESRCH;
>+
>+    rc = -EINVAL;

(Not used anywhere up from here.)

>+    if ( is_pvh_domain(d)
>+         && (a.index != HVM_PARAM_CALLBACK_IRQ) )
>+        goto out;

It would have been nice if you had corrected style issues like the misplaced &&
as you go; I'll try to remember to do so while committing (together with a few more
and the adjustment for the issue above).

>+    case HVM_PARAM_IOREQ_PFN:
>+    case HVM_PARAM_BUFIOREQ_PFN:
>+    case HVM_PARAM_BUFIOREQ_EVTCHN:
>+    {
>+        domid_t domid;
>+
>+        /* May need to create server */
>+        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
>+        rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL);
>+        if ( rc != 0 && rc != -EEXIST )
>+            goto out;
>+        /*FALLTHRU*/
>+    }
>+    default:

Andrew - will Coverity be happy with the fall-through comment being followed
by a closing brace?

Jan

Re: [PATCH v2 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions

[Andrew Cooper]

On 01/05/15 20:19, Jan Beulich wrote:
>>>> Paul Durrant <paul.durrant@citrix.com> 05/01/15 4:05 PM >>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5638,6 +5638,299 @@ static int hvmop_set_evtchn_upcall_vector(
>> return 0;
>> }
>   >
>> +static int hvmop_set_param(
>> +    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
>> +{
>> +    struct domain *curr_d = current->domain;
>> +    struct xen_hvm_param a;
>> +    struct domain *d;
>> +    struct vcpu *v;
>> +    int rc = 0;
> Iirc Andrew indicated that Coverity would complain about dead initializers like
> this.

Likely, yes.

It is less clear whether this case would trigger it, but I know for 
certain that it dislikes "int rc = 0; rc = fn();"

>
>> +    if ( copy_from_guest(&a, arg, 1) )
>> +        return -EFAULT;
>> +
>> +    if ( a.index >= HVM_NR_PARAMS )
>> +        return -EINVAL;
>> +
>> +    d = rcu_lock_domain_by_any_id(a.domid);
>> +    if ( d == NULL )
>> +        return -ESRCH;
>> +
>> +    rc = -EINVAL;
> (Not used anywhere up from here.)
>
>> +    if ( is_pvh_domain(d)
>> +         && (a.index != HVM_PARAM_CALLBACK_IRQ) )
>> +        goto out;
> It would have been nice if you had corrected style issues like the misplaced &&
> as you go; I'll try to remember to do so while committing (together with a few more
> and the adjustment for the issue above).
>
>> +    case HVM_PARAM_IOREQ_PFN:
>> +    case HVM_PARAM_BUFIOREQ_PFN:
>> +    case HVM_PARAM_BUFIOREQ_EVTCHN:
>> +    {
>> +        domid_t domid;
>> +
>> +        /* May need to create server */
>> +        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
>> +        rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL);
>> +        if ( rc != 0 && rc != -EEXIST )
>> +            goto out;
>> +        /*FALLTHRU*/
>> +    }
>> +    default:
> Andrew - will Coverity be happy with the fall-through comment being followed
> by a closing brace?

I don't have the exact reference to hand (I am still travelling), but I 
expect not.  Such a brace could be part of an if/else clause which would 
mean that it wasn't the only codepath to fall through.

~Andrew

>
> Jan
>

Re: [PATCH v2 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:jbeulich@suse.com]
> Sent: 01 May 2015 20:19
> To: Andrew Cooper; Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org)
> Subject: Re: [PATCH v2 1/3] x86/hvm: give HVMOP_set_param and
> HVMOP_get_param their own functions
> 
> >>> Paul Durrant <paul.durrant@citrix.com> 05/01/15 4:05 PM >>>
> >--- a/xen/arch/x86/hvm/hvm.c
> >+++ b/xen/arch/x86/hvm/hvm.c
> >@@ -5638,6 +5638,299 @@ static int hvmop_set_evtchn_upcall_vector(
> >return 0;
> >}
>  >
> >+static int hvmop_set_param(
> >+    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
> >+{
> >+    struct domain *curr_d = current->domain;
> >+    struct xen_hvm_param a;
> >+    struct domain *d;
> >+    struct vcpu *v;
> >+    int rc = 0;
> 
> Iirc Andrew indicated that Coverity would complain about dead initializers like
> this.

Yes, it may. I'll ditch it in v3.

> 
> >+    if ( copy_from_guest(&a, arg, 1) )
> >+        return -EFAULT;
> >+
> >+    if ( a.index >= HVM_NR_PARAMS )
> >+        return -EINVAL;
> >+
> >+    d = rcu_lock_domain_by_any_id(a.domid);
> >+    if ( d == NULL )
> >+        return -ESRCH;
> >+
> >+    rc = -EINVAL;
> 
> (Not used anywhere up from here.)
> 
> >+    if ( is_pvh_domain(d)
> >+         && (a.index != HVM_PARAM_CALLBACK_IRQ) )
> >+        goto out;
> 
> It would have been nice if you had corrected style issues like the misplaced
> &&
> as you go; I'll try to remember to do so while committing (together with a
> few more
> and the adjustment for the issue above).

I'm happy to do it. Just missed it.

> 
> >+    case HVM_PARAM_IOREQ_PFN:
> >+    case HVM_PARAM_BUFIOREQ_PFN:
> >+    case HVM_PARAM_BUFIOREQ_EVTCHN:
> >+    {
> >+        domid_t domid;
> >+
> >+        /* May need to create server */
> >+        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> >+        rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL);
> >+        if ( rc != 0 && rc != -EEXIST )
> >+            goto out;
> >+        /*FALLTHRU*/
> >+    }
> >+    default:
> 
> Andrew - will Coverity be happy with the fall-through comment being
> followed
> by a closing brace?
> 

It looks wrong anyway. I'll move to after the brace.

  Paul

> Jan

[PATCH v2 2/3] x86/hvm: introduce functions for HVMOP_get/set_param allowance checks

[Paul Durrant]

Some parameters can only (validly) be set once. Some cannot be set
by a guest for its own domain. 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@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |  142 +++++++++++++++++++++++++++---------------------
 1 file changed, 79 insertions(+), 63 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ab71202..1025681 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5638,6 +5638,57 @@ 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;
+
+    /* The following parameters cannot be set by the guest. */
+    switch ( a->index )
+    {
+    case HVM_PARAM_IDENT_PT:
+    case HVM_PARAM_DM_DOMAIN:
+    case HVM_PARAM_ACPI_S_STATE:
+    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;
+
+    /* The following parameters can only be changed once. */
+    switch ( a->index )
+    {
+    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)
 {
@@ -5665,7 +5716,7 @@ static int hvmop_set_param(
          && (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;
 
@@ -5680,31 +5731,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;
@@ -5732,22 +5763,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);
@@ -5760,20 +5781,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;
@@ -5806,22 +5816,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 )
         {
@@ -5861,10 +5861,33 @@ 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;
+
+    /* The following parameters should not be read by the guest. */
+    switch ( a->index )
+    {
+    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 = 0;
@@ -5887,7 +5910,7 @@ static int hvmop_get_param(
          && (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;
 
@@ -5896,13 +5919,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

Re: [PATCH v2 2/3] x86/hvm: introduce functions for HVMOP_get/set_param allowance checks

[Jan Beulich]

>>> Paul Durrant <paul.durrant@citrix.com> 05/01/15 4:05 PM >>>
>+    /* The following parameters cannot be set by the guest. */

Didn't you agree that "cannot" isn't the right term here?

>+    /* The following parameters can only be changed once. */
>+    switch ( a->index )
>+    {
>+    case HVM_PARAM_VIRIDIAN:
>+    case HVM_PARAM_IOREQ_SERVER_PFN:
>+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>+        if (value != 0 && a->value != value)

There are still blanks missing here.

>case HVM_PARAM_IDENT_PT:
>-        /* Not reflexive, as we must domain_pause(). */

And I continue to think that retaining these comments would be useful for
documentation purposes (even when switching to white lists - they should
then remain explicit case labels grouped with the "default" one). And taking
into consideration the comment change requested at the very top (which
changes again in the 3rd patch), there could then perhaps be a distinction
between "can't", "shouldn't", and "mustn't" (again solely serving doc
purposes).

Jan

Re: [PATCH v2 2/3] x86/hvm: introduce functions for HVMOP_get/set_param allowance checks

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:jbeulich@suse.com]
> Sent: 01 May 2015 20:28
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org)
> Subject: Re: [PATCH v2 2/3] x86/hvm: introduce functions for
> HVMOP_get/set_param allowance checks
> 
> >>> Paul Durrant <paul.durrant@citrix.com> 05/01/15 4:05 PM >>>
> >+    /* The following parameters cannot be set by the guest. */
> 
> Didn't you agree that "cannot" isn't the right term here?
> 

Yes, sorry I must have missed that.

> >+    /* The following parameters can only be changed once. */
> >+    switch ( a->index )
> >+    {
> >+    case HVM_PARAM_VIRIDIAN:
> >+    case HVM_PARAM_IOREQ_SERVER_PFN:
> >+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> >+        if (value != 0 && a->value != value)
> 
> There are still blanks missing here.
> 

True.

> >case HVM_PARAM_IDENT_PT:
> >-        /* Not reflexive, as we must domain_pause(). */
> 
> And I continue to think that retaining these comments would be useful for
> documentation purposes (even when switching to white lists - they should
> then remain explicit case labels grouped with the "default" one). And taking
> into consideration the comment change requested at the very top (which
> changes again in the 3rd patch), there could then perhaps be a distinction
> between "can't", "shouldn't", and "mustn't" (again solely serving doc
> purposes).
> 

Ok, if you don't mind have the extra functionally redundant case statements.

  Paul

> Jan

Re: [PATCH v2 2/3] x86/hvm: introduce functions for HVMOP_get/set_param allowance checks

[Jan Beulich]

>>> On 05.05.15 at 11:25, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:jbeulich@suse.com]
>> Sent: 01 May 2015 20:28
>> And I continue to think that retaining these comments would be useful for
>> documentation purposes (even when switching to white lists - they should
>> then remain explicit case labels grouped with the "default" one). And taking
>> into consideration the comment change requested at the very top (which
>> changes again in the 3rd patch), there could then perhaps be a distinction
>> between "can't", "shouldn't", and "mustn't" (again solely serving doc
>> purposes).
>> 
> 
> Ok, if you don't mind have the extra functionally redundant case statements.

Yes, I certainly don't mind those if they serve a purpose (documentation).

Jan

[PATCH v2 3/3] x86/hvm: Use white-lists for HVM param guest accessibility checks

[Paul Durrant]

There are actually very few HVM parameters that a guest needs to read
and even fewer that a guest needs to write. Use white-lists to specify
those parameters and also ensre that, by default, newly introduced
parameters are not accessible.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |   40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 1025681..3725349 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5648,25 +5648,18 @@ static int hvm_allow_set_param(struct domain *d,
     if ( rc )
         return rc;
 
-    /* The following parameters cannot be set by the guest. */
+    /* The following parameters can be set by the guest. */
     switch ( a->index )
     {
-    case HVM_PARAM_IDENT_PT:
-    case HVM_PARAM_DM_DOMAIN:
-    case HVM_PARAM_ACPI_S_STATE:
-    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;
+    case HVM_PARAM_CALLBACK_IRQ:
+    case HVM_PARAM_VM86_TSS:
+    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
+    case HVM_PARAM_TRIPLE_FAULT_REASON:
+    case HVM_PARAM_VM_GENERATION_ID_ADDR:
         break;
     default:
+        if ( d == current->domain )
+            rc = -EPERM;
         break;
     }
 
@@ -5870,15 +5863,22 @@ static int hvm_allow_get_param(struct domain *d,
     if ( rc )
         return rc;
 
-    /* The following parameters should not be read by the guest. */
+    /* The following parameters can be read by the guest. */
     switch ( a->index )
     {
-    case HVM_PARAM_IOREQ_SERVER_PFN:
-    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
-        if ( d == current->domain )
-            rc = -EPERM;
+    case HVM_PARAM_CALLBACK_IRQ:
+    case HVM_PARAM_VM86_TSS:
+    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
+    case HVM_PARAM_TRIPLE_FAULT_REASON:
+    case HVM_PARAM_VM_GENERATION_ID_ADDR:
+    case HVM_PARAM_STORE_PFN:
+    case HVM_PARAM_STORE_EVTCHN:
+    case HVM_PARAM_CONSOLE_PFN:
+    case HVM_PARAM_CONSOLE_EVTCHN:
         break;
     default:
+        if ( d == current->domain )
+            rc = -EPERM;
         break;
     }
 
-- 
1.7.10.4