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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 05 September 2018 19:12
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a
> whitelist
> 
> There are holes in the HVM_PARAM space, some of which are from
> deprecated
> parameters, but toolstack and device models currently has (almost) blanket

s/has/have

> write access.
> 
> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-writeable

s/get/set

> 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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  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. */

Personally I'm finding this indentation of comment incongruous. Such comments are logically outside of any particular case statement so I think they should have the same level of indentation as the case statements themselves.

>      case HVM_PARAM_CALLBACK_IRQ:
>      case HVM_PARAM_VM86_TSS:
>      case HVM_PARAM_VM86_TSS_SIZED:
> @@ -4083,18 +4083,40 @@ static int hvm_allow_set_param(struct domain
> *d,
>      case HVM_PARAM_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:
> +    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;
> -

Again, I think an ASSERT here would be good.

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

Does anything rely on this EOPNOTSUPP vs the EINVAL that would be returned after this patch is applied?

  Paul

> +
>      case HVM_PARAM_NESTEDHVM:
>          rc = xsm_hvm_param_nested(XSM_PRIV, d);
>          if ( rc )
> @@ -4253,9 +4264,7 @@ static int hvmop_set_param(
>               d->arch.hvm.params[HVM_PARAM_NESTEDHVM] )
>              rc = -EINVAL;
>          break;
> -    case HVM_PARAM_BUFIOREQ_EVTCHN:
> -        rc = -EINVAL;
> -        break;
> +
>      case HVM_PARAM_TRIPLE_FAULT_REASON:
>          if ( a.value > SHUTDOWN_MAX )
>              rc = -EINVAL;
> --
> 2.1.4

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