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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/05/15 11:25, Paul Durrant wrote:
> 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@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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