Re: [Xen-devel] [PATCH 2/7] vm-event: VM_EVENT_FLAG_DENY requires VM_EVENT_FLAG_VCPU_PAUSED

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 6/21/2016 6:09 PM, Tamas K Lengyel wrote:

On Jun 21, 2016 05:26, "Corneliu ZUZU" <czuzu@xxxxxxxxxxxxxxx> wrote:
>
> On 6/16/2016 7:11 PM, Tamas K Lengyel wrote:
>>
>> On Thu, Jun 16, 2016 at 8:07 AM, Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx> wrote:
>>>
>>> For VM_EVENT_FLAG_DENY to work, the vcpu must be paused (sync = 1) until the
>>> vm-event is handled. A vm-event response having VM_EVENT_FLAG_DENY flag set
>>> should also set the VM_EVENT_FLAG_VCPU_PAUSED flag. Enforce that in
>>> vm_event_register_write_resume().
>>
>> Well, the problem with this is that the user can set the VCPU_PAUSED
>> flag any time it wants. It can happen that Xen hasn't paused the vCPU
>> but the user still sends that flag, in which case the unpause the flag
>> induces will not actually do anything. You should also check if the
>> vCPU is in fact paused rather then just relying on this flag.
>>
>> Tamas
>>
>
> Tamas,
>
> Isn't that also the case with the following block @ vm_event_resume:
>
>         if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
>         {
>             if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
>                 vm_event_set_registers(v, &rsp);
>
>             if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
>                 vm_event_toggle_singlestep(d, v);
>
>             vm_event_vcpu_unpause(v);
>         }
>
> , i.e. shouldn't it be fixed to:
>
>         /* check flags which apply only when the vCPU is paused */
>         if ( atomic_read(&v->vm_event_pause_count) )
>         {
>             if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
>                 vm_event_set_registers(v, &rsp);
>             if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
>                 vm_event_toggle_singlestep(d, v);
>             if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
>                 vm_event_vcpu_unpause(v);
>         }
> ?
>
> If this holds, the check for vCPU pause can also be removed from vm_event_toggle_singlestep (maybe turned into an ASSERT which could also be added to vm_event_set_registers).
>

Yes, reworking that whole part as you outlined above would be nice!

Tamas

I've also noticed there's a vm-event vCPU pause count (v->vm_event_pause_count), which is synchronized with the global pause count (v->pause_count).
Since we rely on vm_event_pause_count to determine if the vCPU is paused, I'm wondering if it can't be 'corrupted' by the user.

More specifically, is the code written to ensure that the following can't happen:
1. v->vm_event_pause_count > 0 (vm-event subsystem reports that the vCPU is paused)
2. the toolstack user can somehow decrement the global v->pause_count without decrementing v->vm_event_pause_count (maybe by doing consecutive domain resumes)
3. after many decrements by the user (2.),  v->pause_count = 0 (the vCPU becomes unpaused), but v->vm_event_pause_count is still > 0
?

Apparently it can't since there's a separate per-domain pause_count as well, but the code is a bit involved so I'm just asking to be sure.

Corneliu.

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