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

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

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 06 September 2018 11:37
> To: Paul Durrant <Paul.Durrant@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@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
> 
> 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.

Ah, true.

> 
> 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.
> 

Yes, that's where I meant to suggest adding the ASSERT. I wanted it to verify that sanitization.

  Paul

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel