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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/06/2015 08:05 PM, Lengyel, Tamas wrote:
> @@ -410,6 +414,8 @@ void vm_event_resume(struct domain *d, struct
> vm_event_domain *ved)
> 
> 
>      #ifdef HAS_MEM_ACCESS
>              case VM_EVENT_REASON_MEM_ACCESS:
>     +        case VM_EVENT_REASON_MOV_TO_MSR:
>     +        case VM_EVENT_REASON_WRITE_CTRLREG:
> 
> 
> This doesn't really make much sense to be associated with MEM_ACCESS.
> I'm adding a separate arch-specific vm_event file in my other singlestep
> patch, I think these should trigger their appropriate handler there, not
> in mem_access_resume.

To be honest I've somewhat anticipated this critique, and it's
definitely valid. I just wasn't sure if somebody would have said that
it's better to process all responses in one place for now, so I've
decided to try it this way first. But yes, that's actually my preference
as well, so no problem.

>                  mem_access_resume(v, &rsp);
>                  break;
>      #endifdiff --git a/xen/include/public/vm_event.h
>     b/xen/include/public/vm_event.h
>     index f0da008..bc97334 100644
>     --- a/xen/include/public/vm_event.h
>     +++ b/xen/include/public/vm_event.h
>     @@ -158,6 +158,11 @@ struct vm_event_regs_x86 {
>       * MEM_ACCESS_EMULATE_NOWRITE.
>       */
>      #define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
>     + /*
>     +  * Deny completion of the operation that triggered the event.
>     +  * Currently only useful for MSR, CR0, CR3 and CR4 write events.
>     +  */
>     +#define MEM_ACCESS_DENY                 (1 << 9)
> 
> 
> Same comment here, this feature is not really denying a mem_access, it
> denies register writes. Associating it with mem_access just makes it
> confusing. IMHO defining it as a VM_EVENT_FLAG_DENY_REGISTER_CHANGE or
> something similar instead would make it a lot more descriptive and
> inline with that it is actually doing.

Fair enough, I'll think of a more appropriate name for it.


Thanks,
Razvan