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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.05.15 at 22:45, <rcojocaru@xxxxxxxxxxxxxxx> 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).
> 
> 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.

Considering that in the reply to Andrew's response you already
pointed at where a suggestion towards consistent pre events was
made, it would have helped if you identified where this was advised
against before. It certainly seems sensible to treat MSR and CR
writes in a consistent fashion.

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

If the setting is being done by a monitor VM, I would suppose
v != current, and hence - along the lines above - no need for an
event. Whereas when v == current, the setting would be done
by the VM itself, and hence an event should always be delivered.

Jan


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