Re: [Xen-devel] [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 05 September 2018 19:12
> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> <JBeulich@xxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
> Subject: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's
> 
> ARM currently has no restrictions on toolstack and guest access to the entire
> HVM_PARAM block.  As the paging/monitor/sharing features aren't under
> security
> support, this doesn't need an XSA.
> 
> The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details exposed
> read-only to
> the guest, while the *_RING_PFN details are restricted to only toolstack
> access.  No other parameters are used.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@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>
> 
> This is only compile tested, and based on my reading of the source.  There
> might be other PARAMS needing including.
> ---
>  xen/arch/arm/hvm.c | 62
> +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 76b27c9..3581ba2 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -31,6 +31,57 @@
> 
>  #include <asm/hypercall.h>
> 
> +static int hvm_allow_set_param(const struct domain *d, unsigned int
> param)
> +{
> +    switch ( param )
> +    {
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * They may not be set by the domain.
> +         */
> +    case HVM_PARAM_CALLBACK_IRQ:
> +    case HVM_PARAM_STORE_PFN:
> +    case HVM_PARAM_STORE_EVTCHN:
> +    case HVM_PARAM_CONSOLE_PFN:
> +    case HVM_PARAM_CONSOLE_EVTCHN:

Probably should remove the EVTCHN params from this list after fixing patch #3.

> +    case HVM_PARAM_PAGING_RING_PFN:
> +    case HVM_PARAM_MONITOR_RING_PFN:
> +    case HVM_PARAM_SHARING_RING_PFN:
> +        return d == current->domain ? -EPERM : 0;
> +
> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
> +    default:
> +        return -EINVAL;
> +    }
> +}
> +
> +static int hvm_allow_get_param(const struct domain *d, unsigned int
> param)
> +{
> +    switch ( param )
> +    {
> +        /* The following parameters can be read by the guest and toolstack. 
> */
> +    case HVM_PARAM_CALLBACK_IRQ:
> +    case HVM_PARAM_STORE_PFN:
> +    case HVM_PARAM_STORE_EVTCHN:
> +    case HVM_PARAM_CONSOLE_PFN:
> +    case HVM_PARAM_CONSOLE_EVTCHN:
> +        return 0;
> +
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * They may not be read by the domain.
> +         */
> +    case HVM_PARAM_PAGING_RING_PFN:
> +    case HVM_PARAM_MONITOR_RING_PFN:
> +    case HVM_PARAM_SHARING_RING_PFN:
> +        return d == current->domain ? -EPERM : 0;
> +
> +        /* Hole, deprecated, or out-of-range. */
> +    default:
> +        return -EINVAL;
> +    }
> +}
> +
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
> arg)
>  {
>      long rc = 0;
> @@ -46,9 +97,6 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_from_guest(&a, arg, 1) )
>              return -EFAULT;
> 
> -        if ( a.index >= HVM_NR_PARAMS )
> -            return -EINVAL;
> -

ASSERT here.

  Paul

>          d = rcu_lock_domain_by_any_id(a.domid);
>          if ( d == NULL )
>              return -ESRCH;
> @@ -59,10 +107,18 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> 
>          if ( op == HVMOP_set_param )
>          {
> +            rc = hvm_allow_set_param(d, a.index);
> +            if ( rc )
> +                goto param_fail;
> +
>              d->arch.hvm.params[a.index] = a.value;
>          }
>          else
>          {
> +            rc = hvm_allow_get_param(d, a.index);
> +            if ( rc )
> +                goto param_fail;
> +
>              a.value = d->arch.hvm.params[a.index];
>              rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>          }
> --
> 2.1.4

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