[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Paul,

On 06/09/18 10:29, Paul Durrant wrote:
-----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.

I don't think this would be correct. This is an input from the guest, so if you do fuzzing you will end up to get an hypervisor crash rather than returning an error.

A potential place for an ASSERT would be just before accessing hvm.params. But then, technically the index should have been sanitized by hvm_allow_{get,set}_param.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.