Re: [Xen-devel] [PATCH v3 3/3] x86/hvm: Use white-lists for HVM param guest accessibility checks

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 05 May 2015 11:54
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Keir (Xen.org); Jan Beulich
> Subject: Re: [PATCH v3 3/3] x86/hvm: Use white-lists for HVM param guest
> accessibility checks
> 
> On 05/05/15 11:25, Paul Durrant wrote:
> > 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@xxxxxxxxxx>
> > Cc: Keir Fraser <keir@xxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/hvm.c |   39 ++++++++++++++++++++++-----------------
> >  1 file changed, 22 insertions(+), 17 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 03543dd..ccf19a4 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -5650,6 +5650,13 @@ static int hvm_allow_set_param(struct domain
> *d,
> >
> >      switch ( a->index )
> >      {
> > +    /* The following parameters can be set by the guest. */
> > +    case HVM_PARAM_CALLBACK_IRQ:
> > +    case HVM_PARAM_VM86_TSS:
> 
> The only case where the VM86_TSS is needed is when VT-x doesn't support
> unrestricted mode, in which case this parameter and IDENT_PT must be set
> up by the domain builder in order to execute hvmloader.  Neither need to
> be settable by the guest.

Really? What about the hvm_param_set() done at hvmloader.c:183? 

> 
> > +    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
> > +    case HVM_PARAM_TRIPLE_FAULT_REASON:
> 
> TRIPLE_FAULT_REASON really shouldn't be settable by the guest (although
> it is not a security problem that it currently is - it defaults to the
> most expensive toolstack action possible).
> 
> Its intended purpose was to catch triple faults as crashes rather than
> reboots (i.e. a difference from real hardware).
> 

Ok. I'll shoot that one then.

> > +    case HVM_PARAM_VM_GENERATION_ID_ADDR:
> > +        break;
> >      /*
> >       * The following parameters must not be set by the guest
> >       * since the domain may need to be paused.
> > @@ -5657,21 +5664,11 @@ static int hvm_allow_set_param(struct domain
> *d,
> >      case HVM_PARAM_IDENT_PT:
> >      case HVM_PARAM_DM_DOMAIN:
> >      case HVM_PARAM_ACPI_S_STATE:
> 
> I think you can safely elide the above cases into default.  All that
> matters in this case is that none of the whitelisted parameters need to
> pause the domain.
> 

I think you and Jan differ there, unless I misunderstood Jan.

> > -    /* 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:
> > +    /* The remaining parameters should not be set by the guest. */
> > +    default:
> >          if ( d == current->domain )
> >              rc = -EPERM;
> >          break;
> > -    default:
> > -        break;
> >      }
> >
> >      if ( rc )
> > @@ -5874,6 +5871,17 @@ static int hvm_allow_get_param(struct domain
> *d,
> >
> >      switch ( a->index )
> >      {
> > +    /* The following parameters can be read by the guest. */
> > +    case HVM_PARAM_CALLBACK_IRQ:
> > +    case HVM_PARAM_VM86_TSS:
> > +    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
> 
> The details here should be read out of the ACPI tables, not from an HVM
> Param.
> 

Agreed, but IMO anything that's able to be set by the guest should also be 
readable by the guest.

> > +    case HVM_PARAM_TRIPLE_FAULT_REASON:
> 
> A guest really shouldn't care what action a triple fault will have.
> 

Ok.

> > +    case HVM_PARAM_VM_GENERATION_ID_ADDR:
> 
> Again - should be read from the ACPI tables.
> 

Again, it's set by the guest (or by hvmloader) so it should be readable.

  Paul

> ~Andrew
> 
> > +    case HVM_PARAM_STORE_PFN:
> > +    case HVM_PARAM_STORE_EVTCHN:
> > +    case HVM_PARAM_CONSOLE_PFN:
> > +    case HVM_PARAM_CONSOLE_EVTCHN:
> > +        break;
> >      /*
> >       * The following parameters must not be read by the guest
> >       * since the domain may need to be paused.
> > @@ -5881,14 +5889,11 @@ static int hvm_allow_get_param(struct domain
> *d,
> >      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:
> > +    /* The remaining parameters should not be read by the guest. */
> > +    default:
> >          if ( d == current->domain )
> >              rc = -EPERM;
> >          break;
> > -    default:
> > -        break;
> >      }
> >
> >      return rc;


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