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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

~Andrew

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