Re: [PATCH] x86/hvm: Allow the guest to permit the use of userspace hypercalls

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

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Andrew Cooper
> Sent: 11 January 2016 14:00
> To: Xen-devel
> Cc: Andrew Cooper; Stefano Stabellini; Ian Campbell; Jan Beulich
> Subject: [Xen-devel] [PATCH] x86/hvm: Allow the guest to permit the use of
> userspace hypercalls
> 
> Currently, hypercalls issued from HVM userspace will unconditionally fail with
> -EPERM.
> 
> This is inflexible, and a guest may wish to allow userspace to make
> hypercalls.
> 
> Introduce HVMOP_set_hypercall_dpl which allows the guest to alter the
> permissions check for hypercalls.  It behaves exactly like the dpl field for
> GDT/LDT/IDT entries.
> 
> As the dpl is initialised to 0, hypercalls are restricted to cpl0 code until
> the OS explicitly chooses an alternative.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> --
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> 
> Arm folks: Is something like this sufficiently generic to be useful on Arm,
> perhaps with more generic naming?
> 
> PV guest support for userspace hypercalls is substantially more involved, and
> will take longer to complete.
> ---
>  xen/arch/x86/hvm/hvm.c           | 25 ++++++++++++++++++++++++-
>  xen/include/asm-x86/hvm/domain.h |  2 ++
>  xen/include/public/hvm/hvm_op.h  |  8 ++++++++
>  3 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 21470ec..e5a08db 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5228,7 +5228,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>      case 4:
>      case 2:
>          hvm_get_segment_register(curr, x86_seg_ss, &sreg);
> -        if ( unlikely(sreg.attr.fields.dpl) )
> +        if ( unlikely(sreg.attr.fields.dpl <
> +                      currd->arch.hvm_domain.hypercall_dpl) )

Either I'm going mad, or that check should be '>' shouldn't it? (After all the test is currently effectively a test for > 0)

>          {
>      default:
>              regs->eax = -EPERM;
> @@ -6839,6 +6840,28 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          rc = do_altp2m_op(arg);
>          break;
> 
> +    case HVMOP_set_hypercall_dpl:
> +    {
> +        xen_hvm_hypercall_dpl_t a;
> +        struct domain *d;
> +
> +        if ( copy_from_guest(&a, arg, 1 ) )
> +            return -EFAULT;
> +
> +        rc = rcu_lock_remote_domain_by_id(a.domid, &d);
> +        if ( rc != 0 )
> +            return rc;
> +
> +        if ( current->domain != d )
> +            return -EPERM;
> +
> +        if ( !is_hvm_domain(d) || a.dpl > 3 )
> +            return -EINVAL;
> +
> +        d->arch.hvm_domain.hypercall_dpl = a.dpl;
> +        break;
> +    }
> +
>      default:
>      {
>          gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op);
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-
> x86/hvm/domain.h
> index a8cc2ad..006a142 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -137,6 +137,8 @@ struct hvm_domain {
>      bool_t                 qemu_mapcache_invalidate;
>      bool_t                 is_s3_suspended;
> 
> +    uint32_t               hypercall_dpl;
> +

Why burn 32-bits for a value that's only 2 bits wide?

  Paul

>      /*
>       * TSC value that VCPUs use to calculate their tsc_offset value.
>       * Used during initialization and save/restore.
> diff --git a/xen/include/public/hvm/hvm_op.h
> b/xen/include/public/hvm/hvm_op.h
> index 1606185..f8247db 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -489,6 +489,14 @@ struct xen_hvm_altp2m_op {
>  typedef struct xen_hvm_altp2m_op xen_hvm_altp2m_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_op_t);
> 
> +#define HVMOP_set_hypercall_dpl 26
> +struct xen_hvm_hypercall_dpl {
> +    domid_t domid;
> +    uint16_t dpl;  /* IN[1:0] cpl required to make hypercalls. */
> +};
> +typedef struct xen_hvm_hypercall_dpl xen_hvm_hypercall_dpl_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_hypercall_dpl_t);
> +
>  #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
> 
>  /*
> --
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel