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

[Julien Grall <julien.grall@arm.com>]

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.

Cheers,

-- 
Julien Grall

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