Re: [Xen-devel] [PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a whitelist

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/09/18 09:56, Paul Durrant wrote:
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index c22bf0b..96a6323 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4350,7 +4350,7 @@ static int hvm_allow_get_param(struct domain *d,
>>
>>      switch ( a->index )
>>      {
>> -    /* The following parameters can be read by the guest. */
>> +        /* The following parameters can be read by the guest and toolstack. 
>> */
> Intentional indentation change? I guess so since you do it again below, but 
> just checking.

Yes - this is where BSD style puts comments, as you are inside the
braced scope of the switch statement.

Notice furthermore that there is a correction to the end of the comment.

>
>>      case HVM_PARAM_CALLBACK_IRQ:
>>      case HVM_PARAM_VM86_TSS:
>>      case HVM_PARAM_VM86_TSS_SIZED:
>> @@ -4363,18 +4363,39 @@ static int hvm_allow_get_param(struct domain
>> *d,
>>      case HVM_PARAM_ALTP2M:
>>      case HVM_PARAM_X87_FIP_WIDTH:
>>          break;
>> -    /*
>> -     * The following parameters must not be read by the guest
>> -     * since the domain may need to be paused.
>> -     */
>> +
>> +        /*
>> +         * The following parameters are intended for toolstack usage only.
>> +         * Some require the domain to be paused, and therefore may not read
>> by
>> +         * the domain.
>> +         */
>> +    case HVM_PARAM_PAE_ENABLED:
>>      case HVM_PARAM_IOREQ_PFN:
>>      case HVM_PARAM_BUFIOREQ_PFN:
>>      case HVM_PARAM_BUFIOREQ_EVTCHN:
>> -    /* The remaining parameters should not be read by the guest. */
>> -    default:
>> +    case HVM_PARAM_VIRIDIAN:
>> +    case HVM_PARAM_TIMER_MODE:
>> +    case HVM_PARAM_HPET_ENABLED:
>> +    case HVM_PARAM_IDENT_PT:
>> +    case HVM_PARAM_DM_DOMAIN:
>> +    case HVM_PARAM_ACPI_S_STATE:
>> +    case HVM_PARAM_VPT_ALIGN:
>> +    case HVM_PARAM_NESTEDHVM:
>> +    case HVM_PARAM_PAGING_RING_PFN:
>> +    case HVM_PARAM_MONITOR_RING_PFN:
>> +    case HVM_PARAM_SHARING_RING_PFN:
>> +    case HVM_PARAM_TRIPLE_FAULT_REASON:
>> +    case HVM_PARAM_IOREQ_SERVER_PFN:
>> +    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>> +    case HVM_PARAM_MCA_CAP:
>>          if ( d == current->domain )
>>              rc = -EPERM;
>>          break;
>> +
>> +        /* Hole, deprecated, or out-of-range. */
>> +    default:
>> +        rc = -EINVAL;
>> +        break;
>>      }
>>
>>      return rc;
>> @@ -4390,9 +4411,6 @@ static int hvmop_get_param(
>>      if ( copy_from_guest(&a, arg, 1) )
>>          return -EFAULT;
>>
>> -    if ( a.index >= HVM_NR_PARAMS )
>> -        return -EINVAL;
>> -
> ASSERT, just in case someone screws up the allow function in future?

That's not going to help in any practical way.  This check does really
exist, and is part of the switch statement.

~Andrew

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