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

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

[Andrew Cooper]

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>

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

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

[Konrad Rzeszutek Wilk]

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

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

[Andrew Cooper]

On 11/01/16 17:11, Konrad Rzeszutek Wilk wrote:
> 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?

My specific usecase,
http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen-test-framework.git;a=shortlog;h=refs/heads/wip-traps-v0.1

It isn't quite ready for formal release yet.

> As in why the ioctl via the kernel is no good?

Who says Linux is running?

Hopefully answered in
http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01155.html

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

Correct.  Hypercalls are by default limited to cpl0 (i.e. the existing
behaviour), but guests can use this new hypercall to change the
permission check.

Naturally, you have to be sufficiently privileged to make this hypercall
in the first place, so only the kernel may opt to relax the check.

~Andrew

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

[Konrad Rzeszutek Wilk]

On Mon, Jan 11, 2016 at 05:58:47PM +0000, Andrew Cooper wrote:
> On 11/01/16 17:11, Konrad Rzeszutek Wilk wrote:
> > 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?
> 
> My specific usecase,
> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen-test-framework.git;a=shortlog;h=refs/heads/wip-traps-v0.1
> 
> It isn't quite ready for formal release yet.
> 
> > As in why the ioctl via the kernel is no good?
> 
> Who says Linux is running?

What else would there be :-)

> 
> Hopefully answered in
> http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01155.html

Yes. If you could add it in the commit description that would be most helpful.

Thank you!
> 
> >
> >> 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?
> 
> Correct.  Hypercalls are by default limited to cpl0 (i.e. the existing
> behaviour), but guests can use this new hypercall to change the
> permission check.
> 
> Naturally, you have to be sufficiently privileged to make this hypercall
> in the first place, so only the kernel may opt to relax the check.

Right. Sorry I somehow had in mind that this hypercall would be made by the
toolstack which is why I was confused.
> 
> ~Andrew

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

[George Dunlap]

On Mon, Jan 11, 2016 at 5:58 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 11/01/16 17:11, Konrad Rzeszutek Wilk wrote:
>> 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?
>
> My specific usecase,
> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen-test-framework.git;a=shortlog;h=refs/heads/wip-traps-v0.1
>
> It isn't quite ready for formal release yet.
>
>> As in why the ioctl via the kernel is no good?
>
> Who says Linux is running?
>
> Hopefully answered in
> http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01155.html

Not really.  Obviously if you're running custom test code rather than
Linux, then you aren't going to make an ioctl system call on a file
descriptor; but what people are actually suggesting is just that you
make *some* sort of system call from ring 3 which will then make the
hypercall from ring 0.  That's not "the Linux way" of doing things,
it's the *operating system* way of doing things.

>From the previous discussion, ISTR that what you want to be able to
log messages to the Xen console from your test code when running in
ring 3.  It should be fairly easy to set up a custom system call in
your test system that will then make the appropriate hypercall from
ring 0 and return, with minimal interaction with other parts of the
system.  (I think there were some other suggestions there as well.)

Is there a reason that's not possible?

 -George

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

[Jan Beulich]

>>> On 11.01.16 at 17:51, <andrew.cooper3@citrix.com> 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.
> 
> 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>
> 
> v2:
>  * Fix rcu lock and dpl check.

That's a bold statement considering ...

> @@ -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;
> +    }

... there's no unlock anywhere here.

Jan

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

[Stefano Stabellini]

On Mon, 11 Jan 2016, 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.
> 
> 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>
> 
> 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?

Hypercalls on ARM are made issuing an HVC instruction which is
"UNDEFINED in Secure state, and in User mode in Non-secure state".

In other words, it cannot work.


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