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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 05/18/2015 10:27 AM, Jan Beulich wrote:
>>>> On 15.05.15 at 22:45, <rcojocaru@bitdefender.com> wrote:
>> 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.
> 
> 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.

Sorry for the omission. This is the original reply:

http://lists.xen.org/archives/html/xen-devel/2014-10/msg00240.html

where you've pointed out that we should not break existing behaviour.

After that, Tamas suggested that I could simply write the previous value
back in the vm_event userspace handler, which is how this patch was
born, and Andrew suggested pre-write hooks.

My suggestions at this point are to either:

1. Modify this patch as suggested and resubmit it in the next series
iteration (after we finish with the CR events cleanup).

2. Change the control register events to pre-write and prevent
post-write events in the future.

3. Add new event types for pre-write control register events (a new
index, CR3_PRE_WRITE or something similar).

4. Keep the existing types of control register events but add a new
per-register flag, similar to how the sync flag works, that specifies
whether the event should be pre or post-write. So the libxc
monitor_write_ctrlreg() functions would take and additional bool
parameter (bool pre_write, for example).

In cases 2-4 this patch could probably be dropped, and control register
write preemption could be added to the vm_event reply DENY flag patch.

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

Ack.


Thanks,
Razvan