Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 05/15/2015 06:57 PM, Jan Beulich wrote:
>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
>> that does that.
> 
> But you should also say a word on why this is needed, since things
> worked fine so far without, and enabling the functions to run
> outside of their own vCPU context is not immediately obviously
> correct.

This is a way to undo malicious CR writes. This is achieved for MSR
writes with the deny vm_event response flag patch in this series, but
the CR events are being send after the actual write. In such cases,
while the VCPU is paused before I put a vm_response in the ring, I can
simply write the old value back.

I've brought up the issue in the past, and the consensus, IIRC, was that
I should not alter existing behaviour (post-write events) - so the
alternatives were either to add a new pre-write CR event (which seemed
messy), or this (which seemed less intrusive).

Of course, if it has now become acceptable to reconsider having the CR
vm_events consistently pre-write, the deny patch could be extended to them.

>> -int hvm_set_cr0(unsigned long value)
>> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>  {
>> -    struct vcpu *v = current;
> 
> This change is covered by neither the title nor the description, but
> considering it's you who sends this likely is the meat of the change.
> However, considering that the three calls you add to
> arch_set_info_guest() pass this in as zero, I even more wonder why
> what the title says is needed in the first place.
> 
> I further wonder whether you wouldn't want an event if and only
> if v == current (in which case the flag parameter could be dropped).

It just seemed useless to send out a vm_event in the case you mention,
since presumably the application setting them is very likely the same
one receiving the events (though, granted, it doesn't need to be). So in
that case, it would be pointless to notify itself that it has done what
it knows it's done.

>> @@ -3328,12 +3330,11 @@ int hvm_set_cr3(unsigned long value)
>>      return X86EMUL_UNHANDLEABLE;
>>  }
>>  
>> -int hvm_set_cr4(unsigned long value)
>> +int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>  {
>> -    struct vcpu *v = current;
>>      unsigned long old_cr;
>>  
>> -    if ( value & hvm_cr4_guest_reserved_bits(v, 0) )
>> +    if ( value & hvm_cr4_guest_reserved_bits(v, with_vm_event ? 0 : 1) )
> 
> Why does this depend on with_vm_event? And if indeed correct,
> please simplify to just !with_vm_event.

hvm_cr4_guest_reserved_bits(v, 1) has an ASSERT(v != current) that
crashes the hypervisor in debug mode (surely for a very good reason). If
vm_event is true, then v != current, so there I've tried to avert the crash.


Thanks for the review,
Razvan