From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Li, Xin" Subject: RE: [PATCH v2] Enable SMEP CPU feature support for XEN hypervisor Date: Sat, 4 Jun 2011 02:49:11 +0800 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: xen-devel List-Id: xen-devel@lists.xenproject.org > > Intel new CPU supports SMEP (Supervisor Mode Execution Protection). SME= P > > prevents software operating with CPL < 3 (supervisor mode) from fetchin= g > > instructions from any linear address with a valid translation for which= the > > U/S > > flag (bit 2) is 1 in every paging-structure entry controlling the trans= lation > > for the linear address. > > > > This patch enables SMEP in Xen to protect Xen hypervisor from executing= pv > > guest > > instructions, whose translation paging-structure entries' U/S flags are= all > > set. >=20 > Xin, >=20 > I have attached a modified version of your patch which I believe deals wi= th > a number of issues. I briefly outline the major ones here: >=20 > 1. The initialisation in cpu/common.c is misguided. Features like > set_in_cr4(), setup_clear_cpu_cap(), and cpu_has_xxx, are designed to all= ow > the feature initialisation to happen close in code to where the feature i= s > used. Hence I have moved the initialisation into traps.c:trap_init(). It's the right way to move. But trap_init() is called before Xen does leaf 7.0 detection, thus we also = need to add leaf 7.0 detection in early_cpu_detect to get boot_cpu_data.x86_capabil= ity[7] initialized before trap_init(). > 2. I have renamed the command-line option to 'smep'. Boolean options can = be > inverted by prepending 'no-', or appending '=3D{0,false,off,no}'. We don'= t > generally encode the negative within the base option name string. >=20 Thanks for doing in this better way. > 3. I have pushed interpretation of the pf_type enumeration out to the > callers of spurious_page_fault(). This is because a SMEP fault while Xen = is > executing should *crash Xen*. So that's what I do. Further, when it is a I'm still wondering is it overkill to kill Xen? An evil guest can crash Xe= n? > guest fault, we should domain_crash() not domain_crash_synchronous() (whi= ch > is generally a more dangerous function for Xen's own health). >=20 > More generally, I wonder whether SMEP should be enabled only for guests > (even PV guests) which detect it via CPUID and proactively enable it via > their virtualised CR4? I mean, it is off in real hardware by default for = a > reason: backward compatibility. Furthermore, we only detect spurious page > faults for buggy old PV guests, the rest will get the SMEP fault punted u= p > to them, which seems odd if they don't understand SMEP. What do you think= ? 64bit pv guest can't use SMEP as the kernel code is user accessible. Thus when guest executes it won't trigger SMEP fault. only when Xen executes it will. It's okay to crash Xen (or just the guest). 32bit pv guest should be able to make use of SMEP. When it is from Xen, we crash Xen. When it's is from guest kernel executing user code, we can inject to guest to let it kill the current process. Of course such cas= es the guest should be able to do SMEP handling. We can't consistently handle it for 64bit and 32bit guest. Thanks! -Xin