Re: [Xen-devel] [PATCH V5 09/12] xen/vm_event: Decouple vm_event and mem_access.

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Fri, Feb 13, 2015 at 10:05 PM, Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
> On 13/02/15 16:33, Tamas K Lengyel wrote:
>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>> index f988291..f89361e 100644
>> --- a/xen/common/vm_event.c
>> +++ b/xen/common/vm_event.c
>> @@ -357,6 +357,67 @@ int vm_event_get_response(struct domain *d, struct 
>> vm_event_domain *ved, vm_even
>>      return 1;
>>  }
>>
>> +/*
>> + * Pull all responses from the given ring and unpause the corresponding vCPU
>> + * if required. Based on the response type, here we can also call custom
>> + * handlers.
>> + *
>> + * Note: responses are handled the same way regardless of which ring they
>> + * arrive on.
>> + */
>> +void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
>> +{
>> +    vm_event_response_t rsp;
>> +
>> +    /* Pull all responses off the ring. */
>> +    while ( vm_event_get_response(d, ved, &rsp) )
>> +    {
>> +        struct vcpu *v;
>> +
>> +        if ( rsp.version != VM_EVENT_INTERFACE_VERSION )
>> +        {
>> +            gdprintk(XENLOG_WARNING, "vm_event interface version 
>> mismatch!");
>> +            continue;
>> +        }
>> +
>> +#ifndef NDEBUG
>> +        if ( rsp.flags & VM_EVENT_FLAG_DUMMY )
>> +            continue;
>> +#endif
>> +
>> +        /* Validate the vcpu_id in the response. */
>> +        if ( (rsp.vcpu_id >= d->max_vcpus) || !d->vcpu[rsp.vcpu_id] )
>> +            continue;
>> +
>> +        v = d->vcpu[rsp.vcpu_id];
>> +
>> +        /*
>> +         * In some cases the response type needs extra handling, so here
>> +         * we call the appropriate handlers.
>> +         */
>> +        switch ( rsp.reason )
>> +        {
>> +
>> +#ifdef HAS_MEM_ACCESS
>> +        case VM_EVENT_REASON_MEM_ACCESS:
>> +            mem_access_resume(v, &rsp);
>> +            break;
>> +#endif
>> +
>> +#ifdef HAS_MEM_PAGING
>> +        case VM_EVENT_REASON_MEM_PAGING:
>> +            p2m_mem_paging_resume(d, &rsp);
>> +            break;
>> +#endif
>> +
>
> You need a default clause which captures unknown/invalid responses, logs
> a message for debugging purposes, and ceases any further processing.  It
> is not sensible for an unknown response to unpause the vcpu.

That's how it works today, no validation on the response fields
anywhere. I was also thinking further checking if the response is
arriving on the appropriate ring - we could send arbitrary responses
on all three rings - but I think it is a bit of an overkill.

>
> This will require you to have a whitelist of known reasons which simply
> break.

Sure, that can be done easily.
Tamas

>
> ~Andrew
>
>> +        };
>> +
>> +        /* Unpause domain. */
>> +        if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
>> +            vm_event_vcpu_unpause(v);
>> +    }
>> +}
>> +
>>  void vm_event_cancel_slot(struct domain *d, struct vm_event_domain *ved)
>>  {
>>      vm_event_ring_lock(ved);
>>
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel