Re: [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a whitelist

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

On 07/09/18 17:01, Roger Pau Monné wrote:
> On Wed, Sep 05, 2018 at 07:12:01PM +0100, Andrew Cooper wrote:
>> There are holes in the HVM_PARAM space, some of which are from deprecated
>> parameters, but toolstack and device models currently has (almost) blanket
>> write access.
>>
>> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-writeable
>> parameters, with the default case failing with -EINVAL.  This subsumes the
>> HVM_NR_PARAMS check, as well as the MEMORY_EVENT_* deprecated block, and the
>> BUFIOREQ_EVTCHN Xen-write-only value.
>>
>> No expected change for the defined, in-use params.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@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>
>> ---
>>  xen/arch/x86/hvm/hvm.c | 53 +++++++++++++++++++++++++++++---------------------
>>  1 file changed, 31 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 96a6323..d19ae35 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4073,7 +4073,7 @@ static int hvm_allow_set_param(struct domain *d,
>>  
>>      switch ( a->index )
>>      {
>> -    /* The following parameters can be set by the guest. */
>> +        /* The following parameters can be set by the guest and toolstack. */
>>      case HVM_PARAM_CALLBACK_IRQ:
>>      case HVM_PARAM_VM86_TSS:
> Note sure about the point of letting the guest set the unreal mode
> TSS, but anyway this is not the scope of the patch.

Because hvmloader still sets it up for HVM guests.

Neither you nor Jan took my hints (when doing various related work) that
unifying the PVH and HVM paths in the domain builder (alongside
IDENT_PT) would be a GoodThing(tm).

OTOH, we do now actually have a fairly simple cleanup task which a
student could be guided through doing, which would allow us to remove
guest access to these two params.

>
>>      case HVM_PARAM_VM86_TSS_SIZED:
>> @@ -4083,18 +4083,40 @@ static int hvm_allow_set_param(struct domain *d,
>>      case HVM_PARAM_CONSOLE_EVTCHN:
> Also it's quite weird that we allow the guest to set the console
> evtchn...
>
>>      case HVM_PARAM_X87_FIP_WIDTH:
>>          break;
>> -    /*
>> -     * The following parameters must not be set 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 while others control
>> +         * permissions in Xen, and therefore may not set by the domain.
>> +         */
>> +    case HVM_PARAM_STORE_PFN:
>> +    case HVM_PARAM_PAE_ENABLED:
>> +    case HVM_PARAM_IOREQ_PFN:
>> +    case HVM_PARAM_BUFIOREQ_PFN:
>> +    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:
>> -    /* The remaining parameters should not be set by the guest. */
>> -    default:
>> +    case HVM_PARAM_VPT_ALIGN:
>> +    case HVM_PARAM_CONSOLE_PFN:
> ... but not the console page. I think the guest shouldn't be allowed to
> set any of those.

I see you saw why, but you need to read Paul's reply.

Any user of EVTCHNOP_reset needs to recreate the toolstack event
channels and rewrite *_EVTCHN to allow the next entity (in Paul's
example, the windows crash driver) to start back up correctly.

Of course, the same cant be said for a theoretical GNTTABOP_reset.  (If
it seems like this is a sprawling mess of swamps, that's because its all
terrible.)

>
>> +    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;
>> +
>> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
>> +    default:
>> +        rc = -EINVAL;
>> +        break;
>>      }
>>  
>>      if ( rc )
>> @@ -4130,9 +4152,6 @@ static int hvmop_set_param(
>>      if ( copy_from_guest(&a, arg, 1) )
>>          return -EFAULT;
>>  
>> -    if ( a.index >= HVM_NR_PARAMS )
>> -        return -EINVAL;
>> -
>>      d = rcu_lock_domain_by_any_id(a.domid);
>>      if ( d == NULL )
>>          return -ESRCH;
>> @@ -4209,15 +4228,7 @@ static int hvmop_set_param(
>>      case HVM_PARAM_ACPI_IOPORTS_LOCATION:
>>          rc = pmtimer_change_ioport(d, a.value);
>>          break;
>> -    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:
>> -        /* Deprecated */
>> -        rc = -EOPNOTSUPP;
>> -        break;
> I assume there's no toolstack logic that relies on those parameters
> returning EOPNOTSUPP?

No.  There is a libxc function which intercepts these params and fails
early with -EOPNOTSUPP, so hypercalls never hit Xen, but I can't find
any code which requests these PARAMs now.

~Andrew

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