Re: [Xen-devel] [PATCH v1 4/6] x86/hvm: Add SMAP support to HVM guest

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.04.14 at 15:02, <feng.wu@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -29,6 +29,7 @@
>  #include <xen/sched.h>
>  #include <asm/page.h>
>  #include <asm/guest_pt.h>
> +#include <asm/hvm/vmx/vmx.h>
>  
>  
>  /* Flags that are needed in a pagetable entry, with the sense of NX inverted 
> */
> @@ -144,14 +145,18 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain 
> *p2m,
>      guest_l4e_t *l4p;
>  #endif
>      uint32_t gflags, mflags, iflags, rc = 0;
> -    int smep;
> +    int smep, smap;

These want to be bool_t I suppose.

>      bool_t pse1G = 0, pse2M = 0;
> +    unsigned long sel = 0;
> +    uint64_t eflags = guest_cpu_user_regs()->eflags;
>      p2m_query_t qt = P2M_ALLOC | P2M_UNSHARE;
>  
>      perfc_incr(guest_walk);
>      memset(gw, 0, sizeof(*gw));
>      gw->va = va;
>  
> +    __vmread(GUEST_CS_SELECTOR, &sel);

You're in common code here - please use the proper HVM
abstraction.

> @@ -165,7 +170,21 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>       * whole walk as if it were a user-mode one and then invert the answer. 
> */
>      smep = (is_hvm_vcpu(v) && hvm_smep_enabled(v) 
>              && (pfec & PFEC_insn_fetch) && !(pfec & PFEC_user_mode) );
> -    if ( smep )
> +
> +    /*
> +     * SMAP: kernel-mode data accesses from user-mode mappings should fault
> +     * A fault is considered as a SMAP violation if the following
> +     * conditions come ture:

"true"

> +     *   - X86_CR4_SMAP is set in CR4
> +     *   - An user page is accessed

"A user page ..." afaik.

> +     *   - CPL = 3 or X86_EFLAGS_AC clear set)

Stray closing parenthesis.

> +     *   - Page fault in kernel mode
> +     */
> +    smap = ( is_hvm_vcpu(v) && hvm_smap_enabled(v)
> +            && !(!((sel & 3) == 3) && (eflags & X86_EFLAGS_AC))

!( == ) is better written as ( != ) or, as done elsewhere, ( < ).

> +            && !(pfec & PFEC_user_mode) );
> +
> +    if ( smep || smap )

Again, please fold these are far as possible (is_hvm_vcpu() and
the PFEC_user_mode are common and hence should be done just
once.

> @@ -363,6 +365,16 @@ static inline int hvm_event_pending(struct vcpu *v)
>  #define HVM_CR4_HOST_MASK (mmu_cr4_features & \
>      (X86_CR4_VMXE | X86_CR4_PAE | X86_CR4_MCE))
>  
> +static inline bool_t hvm_cpuid_has_smap(void)
> +{
> +    unsigned int eax = 0, ebx = 0, ecx = 0, edx = 0;
> +    unsigned int leaf = 0x7;
> +
> +    hvm_cpuid(leaf, &eax, &ebx, &ecx, &edx);

Please pass NULL for all outputs you don't need.

> @@ -371,6 +383,7 @@ static inline int hvm_event_pending(struct vcpu *v)
>          X86_CR4_MCE | X86_CR4_PGE | X86_CR4_PCE |       \
>          X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT |           \
>          (cpu_has_smep ? X86_CR4_SMEP : 0) |             \
> +        (hvm_cpuid_has_smap() ? X86_CR4_SMAP : 0) |     \

What's the reason for the asymmetry with SMEP here? Also, did you
verify that v == current in all call paths? And even if you did, passing
v into the function and adding a respective ASSERT() would seem
rather desirable.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel