xen/arch/arm/hvm.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 3 deletions(-)
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.