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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Jan 11, 2016 at 04:51:19PM +0000, Andrew Cooper wrote:
> 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.


Could you explain a bit of the use-case? As in why the ioctl via the kernel
is no good?

> 
> As the dpl is initialised to 0, hypercalls are restricted to cpl0 code until
> the OS explicitly chooses an alternative.

<scratchis his head> So we enable to make hypercalls but then we don't allow
it unless it is in ring 0?

> 
> 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>
> 
> v2:
>  * Fix rcu lock and dpl check.
>  * Use uint8_t for hypercall_dpl and reposition for better packing.
> 
> The test framework (soon to be published officially) how has both positive and
> negative tests to confirm the correct behaviour of this hypercall.
> 
> 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           | 28 +++++++++++++++++++++++++++-
>  xen/include/asm-x86/hvm/domain.h |  2 ++
>  xen/include/public/hvm/hvm_op.h  |  8 ++++++++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 21470ec..5f3be6b 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) )
>          {
>      default:
>              regs->eax = -EPERM;
> @@ -6839,6 +6840,31 @@ 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;
> +
> +        d = rcu_lock_domain_by_any_id(a.domid);
> +        if ( d == NULL )
> +            return -ESRCH;
> +
> +        if ( current->domain != d )
> +            return -EPERM;
> +
> +        if ( !is_hvm_domain(d) )
> +            return -EINVAL;
> +
> +        if ( a.dpl > 3 )
> +            return -EDOM;
> +
> +        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..ac426ce 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -123,6 +123,8 @@ struct hvm_domain {
>      spinlock_t             uc_lock;
>      bool_t                 is_in_uc_mode;
>  
> +    uint8_t                hypercall_dpl;
> +
>      /* Pass-through */
>      struct hvm_iommu       hvm_iommu;
>  
> 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