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

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

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@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 |   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.

> +    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).

> +    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.

> -    /* 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.

> +    case HVM_PARAM_TRIPLE_FAULT_REASON:

A guest really shouldn't care what action a triple fault will have.

> +    case HVM_PARAM_VM_GENERATION_ID_ADDR:

Again - should be read from the ACPI tables.

~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;