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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.06.15 at 11:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 06/26/2015 11:28 AM, Jan Beulich wrote:
>>>>> On 15.06.15 at 11:03, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> @@ -109,7 +109,10 @@ void hvm_event_cr(unsigned int index, unsigned long 
>>> value, unsigned long old)
>>>  
>>>          hvm_event_traps(currad->monitor.write_ctrlreg_sync & 
>>> ctrlreg_bitmask,
>>>                          &req);
>>> +        return 1;
>>>      }
>>> +
>>> +    return 0;
>>>  }
>>>  
>>>  void hvm_event_msr(unsigned int msr, uint64_t value)
>> 
>> Why is knowing whether an event was sent relevant for
>> hvm_event_cr(), but not for hvm_event_msr()?
> 
> MSR events don't honor onchangeonly, so they're always being sent. A CR
> event won't be sent if onchangeonly is true and the register is being
> set to the same value again.
> 
> Since the change prompted the question, I should perhaps add a comment
> to explain that?

Might be worth it.

>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>> @@ -1048,15 +1048,16 @@ static void load_shadow_guest_state(struct vcpu *v)
>>>  
>>>      nvcpu->guest_cr[0] = __get_vvmcs(vvmcs, CR0_READ_SHADOW);
>>>      nvcpu->guest_cr[4] = __get_vvmcs(vvmcs, CR4_READ_SHADOW);
>>> -    hvm_set_cr0(__get_vvmcs(vvmcs, GUEST_CR0));
>>> -    hvm_set_cr4(__get_vvmcs(vvmcs, GUEST_CR4));
>>> -    hvm_set_cr3(__get_vvmcs(vvmcs, GUEST_CR3));
>>> +    hvm_set_cr0(__get_vvmcs(vvmcs, GUEST_CR0), 1);
>>> +    hvm_set_cr4(__get_vvmcs(vvmcs, GUEST_CR4), 1);
>>> +    hvm_set_cr3(__get_vvmcs(vvmcs, GUEST_CR3), 1);
>> 
>> Are you sure there are no dependencies between the individual
>> registers getting set? And what about the order here versus how
>> you carry out the deferred writes in hvm_do_resume()? I don't
>> think any good can come from updating CR3 before CR4...
> 
> Ack, I'll mirror this order in hvm_do_resume().

With the caveat that viewed globally there may not be one single
correct order, namely when also taking MSRs into account.

>>>      control = __get_vvmcs(vvmcs, VM_ENTRY_CONTROLS);
>>>      if ( control & VM_ENTRY_LOAD_GUEST_PAT )
>>>          hvm_set_guest_pat(v, __get_vvmcs(vvmcs, GUEST_PAT));
>>>      if ( control & VM_ENTRY_LOAD_PERF_GLOBAL_CTRL )
>>> -        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, 
>>> __get_vvmcs(vvmcs, GUEST_PERF_GLOBAL_CTRL));
>>> +        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
>>> +                                __get_vvmcs(vvmcs, 
>>> GUEST_PERF_GLOBAL_CTRL), 1);
>> 
>> So one special MSR gets potentially denied writes to - how about
>> all the other ones like PAT (visible above), EFER, etc? And once
>> you send events for multiple ones - how would you know which
>> ones were denied access to be the time to reach hvm_do_resume()?
>> 
>> All the same questions of course apply to nested SVM code too, just
>> that there the problems are leass easily visible from looking at the
>> patch.
> 
> "Regular" MSRs (the ones that go through hvm_msr_write_intercept()) are
> sufficient in all the introspection use cases we've tested. The code
> would of course have the problem you've mentioned if the code would be
> modified to include PAT & friends, but for now this doesn't seem to be
> an issue.
> 
> Should I add a comment, maybe in hvm_do_resume()?

Not sure where it would best go, but explaining that there are
differences between how MSRs get handled seems very necessary,
along with what the differences are and why.

>>> @@ -1249,15 +1250,16 @@ static void load_vvmcs_host_state(struct vcpu *v)
>>>          __vmwrite(vmcs_h2g_field[i].guest_field, r);
>>>      }
>>>  
>>> -    hvm_set_cr0(__get_vvmcs(vvmcs, HOST_CR0));
>>> -    hvm_set_cr4(__get_vvmcs(vvmcs, HOST_CR4));
>>> -    hvm_set_cr3(__get_vvmcs(vvmcs, HOST_CR3));
>>> +    hvm_set_cr0(__get_vvmcs(vvmcs, HOST_CR0), 1);
>>> +    hvm_set_cr4(__get_vvmcs(vvmcs, HOST_CR4), 1);
>>> +    hvm_set_cr3(__get_vvmcs(vvmcs, HOST_CR3), 1);
>>>  
>>>      control = __get_vvmcs(vvmcs, VM_EXIT_CONTROLS);
>>>      if ( control & VM_EXIT_LOAD_HOST_PAT )
>>>          hvm_set_guest_pat(v, __get_vvmcs(vvmcs, HOST_PAT));
>>>      if ( control & VM_EXIT_LOAD_PERF_GLOBAL_CTRL )
>>> -        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, 
>>> __get_vvmcs(vvmcs, HOST_PERF_GLOBAL_CTRL));
>>> +        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
>>> +                                __get_vvmcs(vvmcs, HOST_PERF_GLOBAL_CTRL), 
>>> 1);
>> 
>> Considering these together with the above - do you really want/
>> need to intercept and send events for both host and guest shadow
>> state changes?
> 
> Guest MSRs should suffice indeed. I was just trying to be consistent,
> but no, the host changes should not be necessary.

Aren't you inverting things here? We're talking about the guest's host
vs (L2) guest values here afaict (note the __get_vvmcs() uses).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel