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

[Corneliu ZUZU <czuzu@bitdefender.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3207 bytes --]

On 6/21/2016 6:09 PM, Tamas K Lengyel wrote:
>
>
> On Jun 21, 2016 05:26, "Corneliu ZUZU" <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
> >
> > On 6/16/2016 7:11 PM, Tamas K Lengyel wrote:
> >>
> >> On Thu, Jun 16, 2016 at 8:07 AM, Corneliu ZUZU 
> <czuzu@bitdefender.com <mailto:czuzu@bitdefender.com>> 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.

[-- Attachment #1.2: Type: text/html, Size: 5035 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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