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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/05/2015 21:45, Razvan Cojocaru wrote:
> On 05/15/2015 06:57 PM, Jan Beulich wrote:
>>>>> On 06.05.15 at 19:12, <rcojocaru@xxxxxxxxxxxxxxx> 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).

I always found it curious that some events where post and some pre.

I though I suggested (or at least considered suggesting) at one point
that it would be sensible to select pre/post for event notification. 
Pre comes with an ability to modify the outcome, whereas post is simply
a notification that something happened.

Once again, if you deem this sensible then now is very definitely the
time to do something about it :)

> 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.

(As a side note, attempting to work around an ASSERT is never a valid
option. There will be a reason why it is there in the first place, and
that reason will most likely invalidate whatever train of logic
attempted to work around it in the first place.  The other option is
that the ASSERT is wrong, in which case it should be removed.)

This ASSERT is muddled up with the cpuid handling.  The second parameter
flips between querying the domain cpuid policy, and the host cpuid
policy, as a stopgap solution for some validity checking on domain restore.

I am in the middle (well - more like just started, too many high
priority interrupts) of rewriting cpuid handling from scratch in an
effort to make heterogeneous feature levelling work in a moderately sane
fashion.

As it stands, altering this 0 to some function of with_vm_event is not
valid, as it will change the calculation of which CR4 bits are permitted
to be modified.  Unfortunately, my best suggestion here is to wire up as
much logic as you can, and leave it short circuited with a /*TODO fixme
when domain cpuid handling works properly */ comment.

~Andrew

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