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

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

[Andrew Cooper]

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@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>

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 | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 76b27c9168..1446d4010c 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -31,6 +31,60 @@
 
 #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.
+         *
+         * The {STORE,CONSOLE}_EVTCHN values will need to become read/write if
+         * a new ABI hasn't appeared by the time migration support is added.
+         */
+    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:
+    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 +100,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;
-
         d = rcu_lock_domain_by_any_id(a.domid);
         if ( d == NULL )
             return -ESRCH;
@@ -59,10 +110,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.11.0


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

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

[Julien Grall]

Hi Andrew,

Thank you for stepping up and trying to make HVM_PARAM better :).

On 10/02/2020 18:45, Andrew Cooper wrote:
> 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.

Actually, only monitor is effectively working (yet not security 
supported) on Arm. The two others are x86 specific.

> 
> 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@citrix.com>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> 
> 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 | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 76b27c9168..1446d4010c 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -31,6 +31,60 @@
>   
>   #include <asm/hypercall.h>
>   
> +static int hvm_allow_set_param(const struct domain *d, unsigned int param)
> +{

Should we move the XSM check here too? This is not too important though.

> +    switch ( param )
> +    {
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * They may not be set by the domain.
> +         *
> +         * The {STORE,CONSOLE}_EVTCHN values will need to become read/write if
> +         * a new ABI hasn't appeared by the time migration support is added.

The comment suggests {STORE, CONSOLE}_EVTCHN values should not be 
read/write. But you implement them as read/write. Is it intended?

> +         */
> +    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:
> +    case HVM_PARAM_PAGING_RING_PFN:
> +    case HVM_PARAM_MONITOR_RING_PFN:
> +    case HVM_PARAM_SHARING_RING_PFN:

I would drop HVM_PARAM_PAGING_RING_PFN and HVM_PARAM_SHARING_RING_PFN as 
they are not used by Arm and AFAICT the toolstack will not set them.

> +        return d == current->domain ? -EPERM : 0;
> +

Looking at the list of HVM param, I think you forgot to add 
HVM_PARAM_VM_GENERATION_ID_ADDR.

> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
> +    default:
> +        return -EINVAL;
> +    }
> +}

Cheers,

-- 
Julien Grall

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

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

[Andrew Cooper]

On 19/02/2020 15:53, Julien Grall wrote:
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index 76b27c9168..1446d4010c 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -31,6 +31,60 @@
>>     #include <asm/hypercall.h>
>>   +static int hvm_allow_set_param(const struct domain *d, unsigned
>> int param)
>> +{
>
> Should we move the XSM check here too? This is not too important though.

Currently there is a single XSM call, before the get/set split.  Moving
it in here would create two.

i.e. the current way compiles smaller and will run (fractionally) faster.

>
>> +    switch ( param )
>> +    {
>> +        /*
>> +         * The following parameters are intended for toolstack usage
>> only.
>> +         * They may not be set by the domain.
>> +         *
>> +         * The {STORE,CONSOLE}_EVTCHN values will need to become
>> read/write if
>> +         * a new ABI hasn't appeared by the time migration support
>> is added.
>
> The comment suggests {STORE, CONSOLE}_EVTCHN values should not be
> read/write. But you implement them as read/write. Is it intended?

It is currently read/write only to !SELF (i.e. only the toolstack or
qemu stub).

If it needs to change in the future, it needs to become a separate block
in the switch statement which returns 0 straight away.

Would "read/write to the guest if" be clearer in the comment?

>
>> +         */
>> +    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:
>> +    case HVM_PARAM_PAGING_RING_PFN:
>> +    case HVM_PARAM_MONITOR_RING_PFN:
>> +    case HVM_PARAM_SHARING_RING_PFN:
>
> I would drop HVM_PARAM_PAGING_RING_PFN and HVM_PARAM_SHARING_RING_PFN
> as they are not used by Arm and AFAICT the toolstack will not set them.

Even better.

>
>> +        return d == current->domain ? -EPERM : 0;
>> +
>
> Looking at the list of HVM param, I think you forgot to add
> HVM_PARAM_VM_GENERATION_ID_ADDR.

It is a windows specific thing.  The spec dates from a pre-ARM time, and
while there is nothing obviously x86-only, it does depend on the AML
(via the DSDT or an SSDT).

Either way, its only relevant for migration and/or revert-to-snapshot,
so is of no use to Xen on ARM at this point in time.

~Andrew

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

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

[Tamas K Lengyel]

On Mon, Feb 10, 2020 at 11:46 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> 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.

There is no paging or sharing implementation on ARM anyway. A cc would
have been nice though.

Thanks,
Tamas

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

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

[Andrew Cooper]

On 19/02/2020 16:44, Tamas K Lengyel wrote:
> On Mon, Feb 10, 2020 at 11:46 AM Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> 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.
> There is no paging or sharing implementation on ARM anyway.

Great.

> A cc would have been nice though.

Sorry.  I didn't consider this something other than "clean up the
parameter handling".  It was based on which parameters were referenced
in common or arm specific code.

~Andrew

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

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

[Andrew Cooper]

ARM currently has no restrictions on toolstack and guest access to the entire
HVM_PARAM block.  As the monitor feature isn't under security support, this
doesn't need an XSA.

The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details are only exposed
read-only to the guest, while MONITOR_RING_PFN is restricted to only toolstack
access.  No other parameters are used.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>

This is only compile tested, and based on my reading of the source.  There
might be other PARAMS needing including.

v2:
 * Drop paging/sharing
 * Clarify comment about {STORE,CONSOLE}_EVTCHN
---
 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 76b27c9168..8951b34086 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.
+         *
+         * The {STORE,CONSOLE}_EVTCHN values will need to become read/write to
+         * the guest (not just the toolstack) if a new ABI hasn't appeared by
+         * the time migration support is added.
+         */
+    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:
+    case HVM_PARAM_MONITOR_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_MONITOR_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;
-
         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.11.0


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

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

[Julien Grall]

Hi Andrew,

On 19/02/2020 19:30, Andrew Cooper wrote:
> ARM currently has no restrictions on toolstack and guest access to the entire
> HVM_PARAM block.  As the monitor feature isn't under security support, this
> doesn't need an XSA.
> 
> The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details are only exposed
> read-only to the guest, while MONITOR_RING_PFN is restricted to only toolstack
> access.  No other parameters are used.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Julien Grall <julien@xen.org>

Cheers,

> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> 
> This is only compile tested, and based on my reading of the source.  There
> might be other PARAMS needing including.
> 
> v2:
>   * Drop paging/sharing
>   * Clarify comment about {STORE,CONSOLE}_EVTCHN
> ---
>   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 76b27c9168..8951b34086 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.
> +         *
> +         * The {STORE,CONSOLE}_EVTCHN values will need to become read/write to
> +         * the guest (not just the toolstack) if a new ABI hasn't appeared by
> +         * the time migration support is added.
> +         */
> +    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:
> +    case HVM_PARAM_MONITOR_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_MONITOR_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;
> -
>           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;
>           }
> 

-- 
Julien Grall

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