| Message ID | 1466086077-7518-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 16, 2016 at 8:07 AM, Corneliu ZUZU <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
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> 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 > Right, I actually meant to take a second look over that. I see a situation like that was taken into account in vm_event_vcpu_unpause (comment "prevent underflow of the vcpu pause count.") How can I check for vcpu pause here? Thanks, Corneliu.
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> 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). Corneliu.
On Jun 21, 2016 05:26, "Corneliu ZUZU" <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> 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
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.
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 5635603..75647c4 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -70,7 +70,8 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v) void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp) { - if ( rsp->flags & VM_EVENT_FLAG_DENY ) + if ( (rsp->flags & VM_EVENT_FLAG_DENY) && + (rsp->flags & VM_EVENT_FLAG_VCPU_PAUSED) ) { struct monitor_write_data *w = &v->arch.vm_event->write_data; diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index 586f43b..8f94e20 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -77,6 +77,7 @@ /* * Deny completion of the operation that triggered the event. * Currently only useful for MSR and control-register write events. + * Requires the vCPU to be paused already (synchronous events only). */ #define VM_EVENT_FLAG_DENY (1 << 6) /*
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(). Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/arch/x86/vm_event.c | 3 ++- xen/include/public/vm_event.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)