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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 25 April 2015 01:19
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Keir (Xen.org); Jan Beulich
> Subject: Re: [PATCH 2/3] x86/hvm: introduce functions for
> HVMOP_get/set_param allowance checks
> 
> On 24/04/15 17:21, Paul Durrant wrote:
> > 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.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Cc: Keir Fraser <keir@xxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/hvm.c |  141 +++++++++++++++++++++++++++--------
> -------------
> >  1 file changed, 79 insertions(+), 62 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 87c47b1..23c604d 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -5640,6 +5640,57 @@ static int hvmop_set_evtchn_upcall_vector(
> >      return 0;
> >  }
> >
> > +static int hvm_allow_set_param(struct domain *d,
> > +                               struct xen_hvm_param *a)
> 
> const
>

Good point.
 
> > +{
> > +    uint64_t value = d->arch.hvm_domain.params[a->index];
> > +    int rc = 0;
> 
> No need for the 0 initialiser (and I know Coverity will complain about it).
> 
> > +
> > +    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)
> 
> style
> 

Oops.

> > +    {
> > +    case HVM_PARAM_VIRIDIAN:
> > +    case HVM_PARAM_IDENT_PT:
> > +    case HVM_PARAM_DM_DOMAIN:
> > +    case HVM_PARAM_ACPI_S_STATE:
> > +    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;
> 
> Please can we turn the guest-settable list into a whitelist rather than
> a blacklist.  We have had several XSAs in the past where a guest has
> been able to modify a value it shouldn't have because new params were
> default writable by everyone.
> 

Yes, I think that's probably a good idea.

> > +    }
> > +
> > +    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)
> >  {
> > @@ -5666,7 +5717,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;
> >
> > @@ -5681,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 == current->domain )
> > -            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 == current->domain )
> > -            break;
> > -
> >          rc = -EINVAL;
> >          if ( d->arch.hvm_domain.params[a.index] != 0 )
> >              break;
> > @@ -5733,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 == current->domain )
> > -            break;
> > -
> >          if ( a.value == DOMID_SELF )
> >              a.value = current->domain->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 ( current->domain == d )
> > -            break;
> > -
> >          rc = 0;
> >          if ( a.value == 3 )
> >              hvm_s3_suspend(d);
> > @@ -5761,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 == 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;
> > @@ -5807,22 +5817,12 @@ static int hvmop_set_param(
> >              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 )
> >          {
> > @@ -5866,6 +5866,30 @@ static int hvmop_set_param(
> >      return rc;
> >  }
> >
> > +static int hvm_allow_get_param(struct domain *d,
> > +                               struct xen_hvm_param *a)
> > +{
> > +    int rc = 0;
> > +
> > +    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    /* The following parameters cannot 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;
> 
> Again, this would be safer as a whitelist.
> 

Hmm, maybe. I'll code it that way in v2 but it does seem to go against the 
general idea of HVM params (which AFAICT were originally analogous to 
start_info for PV guests i.e. for passing info from Xen to guests).

  Paul

> ~Andrew
> 
> > +    }
> > +
> > +    return rc;
> > +}
> > +
> >  static int hvmop_get_param(
> >      XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
> >  {
> > @@ -5891,7 +5915,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;
> >
> > @@ -5900,13 +5924,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 == current->domain )
> > -        {
> > -            rc = -EPERM;
> > -            goto out;
> > -        }
> >      case HVM_PARAM_IOREQ_PFN:
> >      case HVM_PARAM_BUFIOREQ_PFN:
> >      case HVM_PARAM_BUFIOREQ_EVTCHN: {


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