Re: [PATCH v2] Enable SMEP CPU feature support for XEN hypervisor

[Keir Fraser <keir.xen@gmail.com>]

On 03/06/2011 20:37, "Li, Xin" <xin.li@intel.com> wrote:

> One last comment from Jan, he suggested to use
>     page_user = (read_cr4() & X86_CR4_SMEP) ? _PAEG_USER : 0;

I effectively have the same X86_CR4_SMEP check at the bottom of
__spurious_page_fault(), replacing your check of cpu_has_smep.

 -- Keir

> thanks!
> -Xin
> 
>> -----Original Message-----
>> From: Keir Fraser [mailto:keir.xen@gmail.com] On Behalf Of Keir Fraser
>> Sent: Saturday, June 04, 2011 3:22 AM
>> To: Li, Xin
>> Cc: xen-devel; Jan Beulich
>> Subject: Re: [PATCH v2] Enable SMEP CPU feature support for XEN hypervisor
>> 
>> Hi Xen,
>> 
>> New patch attached and comments in-line.
>> 
>> On 03/06/2011 19:49, "Li, Xin" <xin.li@intel.com> wrote:
>> 
>>>> 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 allow
>>>> the feature initialisation to happen close in code to where the feature is
>>>> 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_capability[7] initialized before trap_init().
>> 
>> Oooo good point. Fixed in the attached patch by moving SMEP setup into
>> setup.c, explicitly and immediately after identify_cpu(). I was actually in
>> two minds whether to fix this by extending early_cpu_detect(). Overall I
>> decided I don't want to grow e_c_d() if not absolutely necessary. If the CPU
>> setup stuff in setup.c grows too big and ugly I'd rather refactor it another
>> 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
>>> Xen?
>> 
>> An evil guest has to penetrate Xen before it can crash it in this way. If
>> Xen has been subverted, and is lucky enough to notice, what should it do?
>> The only sane answer is to shoot itself in the head. This kind of issue
>> would require immediate developer attention to fix whatever Xen bug had been
>> exploited to trigger SMEP.
>> 
>>> 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
>>> cases
>>> the guest should be able to do SMEP handling.
>> 
>> Haha, give over on this idea that unexplainable behaviour should make you
>> only crash the process/guest. If your behaviour is unexplainable, and you
>> have pretensions of security, you fail-stop.
>> 
>>> We can't consistently handle it for 64bit and 32bit guest.
>> 
>> Well yeah, but that ignores my actual question, which was...
>> """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 up to them, which
>> seems odd if they don't understand SMEP."""
>> 
>> I mean, I know we may as well just hide the feature from PV 64b guests
>> totally. That's obvious. Let's stop talking about PV 64b guests already! The
>> question is: what to do about PV 32b guests?
>> 
>>  -- Keir
>> 
>>> Thanks!
>>> -Xin
>