Re: [Xen-devel] [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a whitelist

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Sep 05, 2018 at 07:12:01PM +0100, Andrew Cooper wrote:
> There are holes in the HVM_PARAM space, some of which are from deprecated
> parameters, but toolstack and device models currently has (almost) blanket
> write access.
> 
> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-writeable
> parameters, with the default case failing with -EINVAL.  This subsumes the
> HVM_NR_PARAMS check, as well as the MEMORY_EVENT_* deprecated block, and the
> BUFIOREQ_EVTCHN Xen-write-only value.
> 
> No expected change for the defined, in-use params.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c | 53 
> +++++++++++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 96a6323..d19ae35 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4073,7 +4073,7 @@ static int hvm_allow_set_param(struct domain *d,
>  
>      switch ( a->index )
>      {
> -    /* The following parameters can be set by the guest. */
> +        /* The following parameters can be set by the guest and toolstack. */
>      case HVM_PARAM_CALLBACK_IRQ:
>      case HVM_PARAM_VM86_TSS:

Note sure about the point of letting the guest set the unreal mode
TSS, but anyway this is not the scope of the patch.

>      case HVM_PARAM_VM86_TSS_SIZED:
> @@ -4083,18 +4083,40 @@ static int hvm_allow_set_param(struct domain *d,
>      case HVM_PARAM_CONSOLE_EVTCHN:

Also it's quite weird that we allow the guest to set the console
evtchn...

>      case HVM_PARAM_X87_FIP_WIDTH:
>          break;
> -    /*
> -     * The following parameters must not be set by the guest
> -     * since the domain may need to be paused.
> -     */
> +
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * Some require the domain to be paused while others control
> +         * permissions in Xen, and therefore may not set by the domain.
> +         */
> +    case HVM_PARAM_STORE_PFN:
> +    case HVM_PARAM_PAE_ENABLED:
> +    case HVM_PARAM_IOREQ_PFN:
> +    case HVM_PARAM_BUFIOREQ_PFN:
> +    case HVM_PARAM_VIRIDIAN:
> +    case HVM_PARAM_TIMER_MODE:
> +    case HVM_PARAM_HPET_ENABLED:
>      case HVM_PARAM_IDENT_PT:
>      case HVM_PARAM_DM_DOMAIN:
>      case HVM_PARAM_ACPI_S_STATE:
> -    /* The remaining parameters should not be set by the guest. */
> -    default:
> +    case HVM_PARAM_VPT_ALIGN:
> +    case HVM_PARAM_CONSOLE_PFN:

... but not the console page. I think the guest shouldn't be allowed to
set any of those.

> +    case HVM_PARAM_NESTEDHVM:
> +    case HVM_PARAM_PAGING_RING_PFN:
> +    case HVM_PARAM_MONITOR_RING_PFN:
> +    case HVM_PARAM_SHARING_RING_PFN:
> +    case HVM_PARAM_TRIPLE_FAULT_REASON:
> +    case HVM_PARAM_IOREQ_SERVER_PFN:
> +    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> +    case HVM_PARAM_MCA_CAP:
>          if ( d == current->domain )
>              rc = -EPERM;
>          break;
> +
> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
> +    default:
> +        rc = -EINVAL;
> +        break;
>      }
>  
>      if ( rc )
> @@ -4130,9 +4152,6 @@ static int hvmop_set_param(
>      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;
> @@ -4209,15 +4228,7 @@ 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:
> -    case HVM_PARAM_MEMORY_EVENT_INT3:
> -    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
> -    case HVM_PARAM_MEMORY_EVENT_MSR:
> -        /* Deprecated */
> -        rc = -EOPNOTSUPP;
> -        break;

I assume there's no toolstack logic that relies on those parameters
returning EOPNOTSUPP?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel