Re: [PATCH v3 3/3] x86/hvm: Use white-lists for HVM param guest accessibility checks

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

On 05/05/15 15:09, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 05 May 2015 11:54
>> To: Paul Durrant; xen-devel@lists.xenproject.org
>> Cc: Keir (Xen.org); Jan Beulich
>> Subject: Re: [PATCH v3 3/3] x86/hvm: Use white-lists for HVM param guest
>> accessibility checks
>>
>> On 05/05/15 11:25, Paul Durrant wrote:
>>> There are actually very few HVM parameters that a guest needs to read
>>> and even fewer that a guest needs to write. Use white-lists to specify
>>> those parameters and also ensre that, by default, newly introduced
>>> parameters are not accessible.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>> Cc: Keir Fraser <keir@xen.org>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>>  xen/arch/x86/hvm/hvm.c |   39 ++++++++++++++++++++++-----------------
>>>  1 file changed, 22 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 03543dd..ccf19a4 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -5650,6 +5650,13 @@ static int hvm_allow_set_param(struct domain
>> *d,
>>>      switch ( a->index )
>>>      {
>>> +    /* The following parameters can be set by the guest. */
>>> +    case HVM_PARAM_CALLBACK_IRQ:
>>> +    case HVM_PARAM_VM86_TSS:
>> The only case where the VM86_TSS is needed is when VT-x doesn't support
>> unrestricted mode, in which case this parameter and IDENT_PT must be set
>> up by the domain builder in order to execute hvmloader.  Neither need to
>> be settable by the guest.
> Really? What about the hvm_param_set() done at hvmloader.c:183? 

Oops - I completely missed that when looking for it. :s  In which case
VM86_TSS does need to be settable.

>
>>> -    /* The following parameters should not be set by the guest. */
>>> -    case HVM_PARAM_VIRIDIAN:
>>> -    case HVM_PARAM_MEMORY_EVENT_CR0:
>>> -    case HVM_PARAM_MEMORY_EVENT_CR3:
>>> -    case HVM_PARAM_MEMORY_EVENT_CR4:
>>> -    case HVM_PARAM_MEMORY_EVENT_INT3:
>>> -    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
>>> -    case HVM_PARAM_MEMORY_EVENT_MSR:
>>> -    case HVM_PARAM_IOREQ_SERVER_PFN:
>>> -    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>>> +    /* The remaining parameters should not be set by the guest. */
>>> +    default:
>>>          if ( d == current->domain )
>>>              rc = -EPERM;
>>>          break;
>>> -    default:
>>> -        break;
>>>      }
>>>
>>>      if ( rc )
>>> @@ -5874,6 +5871,17 @@ static int hvm_allow_get_param(struct domain
>> *d,
>>>      switch ( a->index )
>>>      {
>>> +    /* The following parameters can be read by the guest. */
>>> +    case HVM_PARAM_CALLBACK_IRQ:
>>> +    case HVM_PARAM_VM86_TSS:
>>> +    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
>> The details here should be read out of the ACPI tables, not from an HVM
>> Param.
>>
> Agreed, but IMO anything that's able to be set by the guest should also be readable by the guest.

A reasonable argument.  Ok.

~Andrew