From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest() Date: Mon, 18 May 2015 10:58:45 +0300 Message-ID: <55599BB5.4050306@bitdefender.com> References: <1430932352-4289-1-git-send-email-rcojocaru@bitdefender.com> <1430932352-4289-6-git-send-email-rcojocaru@bitdefender.com> <5556339D020000780007AA16@mail.emea.novell.com> <55565B05.1020509@bitdefender.com> <5559B075020000780007AE96@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5559B075020000780007AE96@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: tim@xen.org, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, jun.nakajima@intel.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, eddie.dong@intel.com, Aravind.Gopalakrishnan@amd.com, suravee.suthikulpanit@amd.com, keir@xen.org, boris.ostrovsky@oracle.com List-Id: xen-devel@lists.xenproject.org On 05/18/2015 10:27 AM, Jan Beulich wrote: >>>> On 15.05.15 at 22:45, wrote: >> On 05/15/2015 06:57 PM, Jan Beulich wrote: >>>>>> On 06.05.15 at 19:12, 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