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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 06 September 2018 11:40
> To: Julien Grall <julien.grall@arm.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: Re: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's
> 
> 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.cooper3@citrix.com]
> >>> Sent: 05 September 2018 19:12
> >>> To: Xen-devel <xen-devel@lists.xen.org>
> >>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> >>> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> >>> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Stefano
> >>> Stabellini <sstabellini@kernel.org>; Julien Grall
> >>> <julien.grall@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.cooper3@citrix.com>
> >>> ---
> >>> CC: Jan Beulich <JBeulich@suse.com>
> >>> CC: Wei Liu <wei.liu2@citrix.com>
> >>> CC: Roger Pau Monné <roger.pau@citrix.com>
> >>> CC: Paul Durrant <paul.durrant@citrix.com>
> >>> CC: Stefano Stabellini <sstabellini@kernel.org>
> >>> CC: Julien Grall <julien.grall@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.
> 

I'm just uneasy about removing a bounds check. If a logic error creeps into an 'allow' function in future then we'd have no protection. An ASSERT would at least make such a logic error obvious.

  Paul

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