From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corneliu ZUZU Subject: Re: [PATCH 2/7] vm-event: VM_EVENT_FLAG_DENY requires VM_EVENT_FLAG_VCPU_PAUSED Date: Wed, 22 Jun 2016 11:34:18 +0300 Message-ID: <1fadf1db-b592-012b-c1dc-a70fa4be4b7f@bitdefender.com> References: <1466085888-7428-1-git-send-email-czuzu@bitdefender.com> <1466086077-7518-1-git-send-email-czuzu@bitdefender.com> <53ff0b6b-0c3a-bca7-b17e-969dd829a807@bitdefender.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7137016545391322860==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Tamas K Lengyel Cc: Andrew Cooper , Jan Beulich , Razvan Cojocaru , Xen-devel List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============7137016545391322860== Content-Type: multipart/alternative; boundary="------------335154985C051C78B96F5E29" This is a multi-part message in MIME format. --------------335154985C051C78B96F5E29 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 6/21/2016 6:09 PM, Tamas K Lengyel wrote: > > > On Jun 21, 2016 05:26, "Corneliu ZUZU" > wrote: > > > > On 6/16/2016 7:11 PM, Tamas K Lengyel wrote: > >> > >> On Thu, Jun 16, 2016 at 8:07 AM, Corneliu ZUZU > > 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. --------------335154985C051C78B96F5E29 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit
On 6/21/2016 6:09 PM, Tamas K Lengyel wrote:


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


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.
--------------335154985C051C78B96F5E29-- --===============7137016545391322860== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============7137016545391322860==--