Re: [PATCH V5 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/14/2015 03:35 PM, Jan Beulich wrote:
>>>> On 13.07.15 at 19:14, <rcojocaru@bitdefender.com> wrote:
>> Changes since V4:
>>  - Rebased the patch to take into account Tamas' "x86/vm_event:
>>    toggle singlestep from vm_event response".
>>  - Moved the management of heap-allocated vm_event-related
>>    domain data to the specific x86 vm_event domain init / cleanup
>>    helpers.
> 
> I would have understood this to mean vm_event_cleanup_domain()
> instead of ...
> 
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -667,6 +667,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>  
>>  void arch_domain_destroy(struct domain *d)
>>  {
>> +    xfree(d->arch.event_write_data);
> 
> ... here. And I see it is being done there, so why also here?

I was not convinced that it's safe to assume that
vm_event_cleanup_domain() always gets called on domain destruction
(please see also the reply to the 1/3 patch review). That's quite likely
a wrong assumption, but if it's not it's safer.

>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -22,11 +22,19 @@
>>  
>>  #include <xen/sched.h>
>>  #include <asm/hvm/hvm.h>
>> +#include <asm/vm_event.h>
>>  
>>  int vm_event_init_domain(struct domain *d)
>>  {
>>      struct vcpu *v;
>>  
>> +    if ( !d->arch.event_write_data )
>> +        d->arch.event_write_data = xzalloc_array(struct monitor_write_data,
>> +                                                 d->max_vcpus);
> 
> Looking at this again I wonder why the data isn't being made part of
> struct arch_vcpu's vm_event sub-structure. That would also address
> the complaint I have here about this not being a guaranteed maximum
> page size runtime allocation.

I think this is just how the initial suggestion was worded, I'll change it.

>> @@ -61,6 +72,38 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
>>      hvm_toggle_singlestep(v);
>>  }
>>  
>> +void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
>> +{
>> +    if ( rsp->flags & VM_EVENT_FLAG_DENY )
>> +    {
>> +        struct monitor_write_data *w =
>> +            &v->domain->arch.event_write_data[v->vcpu_id];
> 
> That would also eliminate this strange construct.

Indeed, I'm not a fan of it either.

>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -74,6 +74,11 @@
>>   * VM_EVENT_FLAG_SET_EMUL_READ_DATA are set, only the latter will be honored).
>>   */
>>  #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 5)
>> + /*
>> +  * Deny completion of the operation that triggered the event.
>> +  * Currently only useful for MSR, CR0, CR3 and CR4 write events.
>> +  */
>> +#define VM_EVENT_FLAG_DENY               (1 << 6)
> 
> Wouldn't this want adding to the get-capabilities sub-op too?

Yes, it's best to add it. Ack.


Thanks,
Razvan