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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 06/26/2015 11:28 AM, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> > @@ -2010,9 +2010,9 @@ static int vmx_cr_access(unsigned long exit_qualification)
>> >      }
>> >      case VMX_CONTROL_REG_ACCESS_TYPE_CLTS: {
>> >          unsigned long old = curr->arch.hvm_vcpu.guest_cr[0];
>> > +        hvm_event_crX(CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
>> >          curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
>> >          vmx_update_guest_cr(curr, 0);
>> > -        hvm_event_crX(CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
>> >          HVMTRACE_0D(CLTS);
>> >          break;
>> >      }
> I suppose it is intentional to ignore a possible deny here? If so,
> shouldn't the be documented by way of a comment?

On second though, I will document it by way of a comment and leave it as
it is, since this is a corner case that would need special handling
(vmx_update_guest_cr(curr, 0), etc. vs how it's now done in
hvm_do_resume(), which would need a new parameter being passed around,
conditional code, etc.), and AFAIK this event is not interesting from a
DENY perspective and so not currently worth the overhead.

Looking at the code again, I'll also fix another mistake: when cutting
and pasting the hvm_event_crX() above to make it a pre-write event to be
consistent, the hvm_event_crX(CR0, curr->arch.hvm_vcpu.guest_cr[0], old)
code became wrong: old == curr->arch.hvm_vcpu.guest_cr[0], whereas
before, curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS before that call.
I'll fix this too in V3.


Thanks,
Razvan