> -----Original Message-----
> From: Julien Grall [mailto:[email protected]]
> Sent: 06 September 2018 11:37
> To: Paul Durrant <[email protected]>; Andrew Cooper
> <[email protected]>; Xen-devel <[email protected]>
> Cc: Jan Beulich <[email protected]>; Wei Liu <[email protected]>; Roger
> Pau Monne <[email protected]>; Stefano Stabellini
> <[email protected]>
> 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:[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'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 <[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.
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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
