Hi Paul, On 06/09/18 10:29, Paul Durrant wrote:
-----Original Message----- From: Andrew Cooper [mailto:[email protected]] Sent: 05 September 2018 19:12 To: Xen-devel <[email protected]> Cc: Andrew Cooper <[email protected]>; Jan Beulich <[email protected]>; Wei Liu <[email protected]>; Roger Pau Monne <[email protected]>; Paul Durrant <[email protected]>; Stefano Stabellini <[email protected]>; Julien Grall <[email protected]> Subject: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM'sARM 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 <[email protected]> --- CC: Jan Beulich <[email protected]> CC: Wei Liu <[email protected]> CC: Roger Pau Monné <[email protected]> CC: Paul Durrant <[email protected]> CC: Stefano Stabellini <[email protected]> CC: Julien Grall <[email protected]> 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 [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
