On 06/09/18 11:36, Julien Grall wrote: > Hi Paul, > > On 06/09/18 10:29, Paul Durrant wrote: >>> -----Original Message----- >>> From: Andrew Cooper [mailto:andrew.coop...@citrix.com] >>> Sent: 05 September 2018 19:12 >>> To: Xen-devel <xen-de...@lists.xen.org> >>> Cc: Andrew Cooper <andrew.coop...@citrix.com>; Jan Beulich >>> <jbeul...@suse.com>; Wei Liu <wei.l...@citrix.com>; Roger Pau Monne >>> <roger....@citrix.com>; Paul Durrant <paul.durr...@citrix.com>; Stefano >>> Stabellini <sstabell...@kernel.org>; Julien Grall >>> <julien.gr...@arm.com> >>> 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.coop...@citrix.com> >>> --- >>> CC: Jan Beulich <jbeul...@suse.com> >>> CC: Wei Liu <wei.l...@citrix.com> >>> CC: Roger Pau Monné <roger....@citrix.com> >>> CC: Paul Durrant <paul.durr...@citrix.com> >>> CC: Stefano Stabellini <sstabell...@kernel.org> >>> CC: Julien Grall <julien.gr...@arm.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 | 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.
Yeah - across all of these ASSERT() requests - using an assert for a boundary check doesn't do anything in the case where it matters most, and in this case, Julien is correct that it is a fully guest-controlled number at this point. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
