From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH V3 1/6] x86/xsaves: enable xsaves/xrstors for pv guest Date: Tue, 11 Aug 2015 11:24:24 +0100 Message-ID: <55C9CD58.8000502@citrix.com> References: <1438739842-31658-1-git-send-email-shuai.ruan@linux.intel.com> <1438739842-31658-2-git-send-email-shuai.ruan@linux.intel.com> <55C24D21.50209@citrix.com> <20150807080008.GA2976@shuai.ruan@linux.intel.com> <55C4A839.8090307@citrix.com> <20150811075039.GA14406@shuai.ruan@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150811075039.GA14406@shuai.ruan@linux.intel.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: Shuai Ruan Cc: kevin.tian@intel.com, wei.liu2@citrix.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, eddie.dong@intel.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jbeulich@suse.com, jun.nakajima@intel.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 11/08/15 08:50, Shuai Ruan wrote: > On Fri, Aug 07, 2015 at 01:44:41PM +0100, Andrew Cooper wrote: >> On 07/08/15 09:00, Shuai Ruan wrote: >>>>> + goto skip; >>>>> + } >>>>> + >>>>> + if ( !guest_kernel_mode(v, regs) || (regs->edi & 0x3f) ) >>>> What does edi have to do with xsaves? only edx:eax are special >>>> according to the manual. >>>> >>> regs->edi is the guest_linear_address >> Whyso? xsaves takes an unconditional memory parameter, not a pointer >> in %rdi. (regs->edi is only correct for ins/outs because the pointer is >> architecturally required to be in %rdi.) > You are right. The linear_address should be decoded from the instruction. >> There is nothing currently in emulate_privileged_op() which does ModRM >> decoding for memory references, nor SIB decoding. xsaves/xrstors would >> be the first such operations. >> >> I am also not sure that adding arbitrary memory decode here is sensible. >> >> In an ideal world, we would have what is currently x86_emulate() split >> in 3 stages. >> >> Stage 1 does straight instruction decode to some internal representation. >> >> Stage 2 does an audit to see whether the decoded instruction is >> plausible for the reason why an emulation was needed. We have had a >> number of security issues with emulation in the past where guests cause >> one instruction to trap for emulation, then rewrite the instruction to >> be something else, and exploit a bug in the emulator. >> >> Stage 3 performs the actions required for emulation. >> >> Currently, x86_emulate() is limited to instructions which might >> legitimately fault for emulation, but with the advent of VM >> introspection, this is proving to be insufficient. With my x86 >> maintainers hat on, I would like to avoid the current situation we have >> with multiple bits of code doing x86 instruction decode and emulation >> (which are all different). >> >> I think the 3-step approach above caters suitably to all usecases, but >> it is a large project itself. It allows the introspection people to >> have a full and complete x86 emulation infrastructure, while also >> preventing areas like the shadow paging from being opened up to >> potential vulnerabilities in unrelated areas of the x86 architecture. >> >> I would even go so far as to say that it is probably ok not to support >> xsaves/xrestors in PV guests until something along the above lines is >> sorted. The first feature in XSS is processor trace which a PV guest >> couldn't use anyway. I suspect the same applies to most of the other > Why PV guest couldn't use precessor trace? After more consideration, Xen should not expose xsaves/xrstors to PV guests at all. >> XSS features, or they wouldn't need to be privileged in the first place. >> > Thanks for your such detail suggestions. > For xsaves/xrstors would also bring other benefits for PV guest such as > saving memory of XSAVE area. If we do not support xsaves/xrstors in PV , > PV guest would lose these benefits. What's your opinions toward this? PV guests running under Xen are exactly the same as regular user processes running under Linux. There is a reason everything covered by xsaves/xrstors is restricted to ring0; it would be a security hole to allow guests to configure the features themselves. Features such as Processor Trace would need a hypercall interface for guests to use. ~Andrew