Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Wu, Feng]

Please ignore this one, the title is incorrect, I will send out the correct one soon!

> -----Original Message-----
> From: Wu, Feng
> Sent: Tuesday, April 15, 2014 9:02 PM
> To: JBeulich@suse.com; Ian.Campbell@citrix.com; xen-devel@lists.xen.org
> Cc: Nakajima, Jun; Dong, Eddie; Wu, Feng
> Subject: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention
> (SMAP) for Xen
> 
> Supervisor Mode Access Prevention (SMAP) is a new security
> feature disclosed by Intel, please refer to the following
> document:
> 
> http://software.intel.com/sites/default/files/319433-014.pdf
> 
> If CR4.SMAP = 1, supervisor-mode data accesses are not allowed
> to linear addresses that are accessible in user mode. If CPL < 3,
> SMAP protections are disabled if EFLAGS.AC = 1. If CPL = 3, SMAP
> applies to all supervisor-mode data accesses (these are implicit
> supervisor accesses) regardless of the value of EFLAGS.AC.
> 
> This patch enables SMAP in Xen to prevent Xen hypervisor from
> accessing pv guest data, whose translation paging-structure
> entries' U/S flags are all set.
> 
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  xen/arch/x86/setup.c             |  9 +++++++++
>  xen/arch/x86/traps.c             | 34
> ++++++++++++++++++++++++++--------
>  xen/include/asm-x86/cpufeature.h |  1 +
>  xen/include/asm-x86/domain.h     |  6 ++++--
>  4 files changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index e9c2c51..09c974d 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -61,6 +61,10 @@ integer_param("maxcpus", max_cpus);
>  static bool_t __initdata disable_smep;
>  invbool_param("smep", disable_smep);
> 
> +/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
> +static bool_t __initdata disable_smap;
> +invbool_param("smap", disable_smap);
> +
>  /* **** Linux config option: propagated to domain0. */
>  /* "acpi=off":    Sisables both ACPI table parsing and interpreter. */
>  /* "acpi=force":  Override the disable blacklist.                   */
> @@ -1279,6 +1283,11 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
>      if ( cpu_has_smep )
>          set_in_cr4(X86_CR4_SMEP);
> 
> +    if ( disable_smap )
> +        setup_clear_cpu_cap(X86_FEATURE_SMAP);
> +    if ( cpu_has_smap )
> +        set_in_cr4(X86_CR4_SMAP);
> +
>      if ( cpu_has_fsgsbase )
>          set_in_cr4(X86_CR4_FSGSBASE);
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index ed4ae2d..9307f10 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1182,11 +1182,12 @@ static int handle_gdt_ldt_mapping_fault(
>  enum pf_type {
>      real_fault,
>      smep_fault,
> +    smap_fault,
>      spurious_fault
>  };
> 
>  static enum pf_type __page_fault_type(
> -    unsigned long addr, unsigned int error_code)
> +    unsigned long addr, struct cpu_user_regs *regs)
>  {
>      unsigned long mfn, cr3 = read_cr3();
>      l4_pgentry_t l4e, *l4t;
> @@ -1194,6 +1195,7 @@ static enum pf_type __page_fault_type(
>      l2_pgentry_t l2e, *l2t;
>      l1_pgentry_t l1e, *l1t;
>      unsigned int required_flags, disallowed_flags, page_user;
> +    unsigned int error_code = regs->error_code;
> 
>      /*
>       * We do not take spurious page faults in IRQ handlers as we do not
> @@ -1270,11 +1272,26 @@ leaf:
>           ((error_code & (PFEC_insn_fetch|PFEC_user_mode)) ==
> PFEC_insn_fetch) )
>          return smep_fault;
> 
> +    /*
> +     * Supervisor Mode Access Prevention (SMAP):
> +     * Disallow supervisor access user-accessible mappings
> +     * A fault is considered as an SMAP violation if the following
> +     * conditions are ture:
> +     *   - X86_CR4_SMAP is set in CR4
> +     *   - An user page is accessed
> +     *   - CPL=3 or X86_EFLAGS_AC is clear
> +     *   - Page fault in kernel mode
> +     */
> +    if ( (read_cr4() & X86_CR4_SMAP) && page_user &&
> +         !(((regs->cs & 0x03) < 3) && (regs->eflags & X86_EFLAGS_AC)) &&
> +         !(error_code & PFEC_user_mode) )
> +        return smap_fault;
> +
>      return spurious_fault;
>  }
> 
>  static enum pf_type spurious_page_fault(
> -    unsigned long addr, unsigned int error_code)
> +    unsigned long addr, struct cpu_user_regs *regs)
>  {
>      unsigned long flags;
>      enum pf_type pf_type;
> @@ -1284,7 +1301,7 @@ static enum pf_type spurious_page_fault(
>       * page tables from becoming invalid under our feet during the walk.
>       */
>      local_irq_save(flags);
> -    pf_type = __page_fault_type(addr, error_code);
> +    pf_type = __page_fault_type(addr, regs);
>      local_irq_restore(flags);
> 
>      return pf_type;
> @@ -1379,8 +1396,8 @@ void do_page_fault(struct cpu_user_regs *regs)
> 
>      if ( unlikely(!guest_mode(regs)) )
>      {
> -        pf_type = spurious_page_fault(addr, error_code);
> -        BUG_ON(pf_type == smep_fault);
> +        pf_type = spurious_page_fault(addr, regs);
> +        BUG_ON((pf_type == smep_fault) || (pf_type == smap_fault));
>          if ( pf_type != real_fault )
>              return;
> 
> @@ -1406,10 +1423,11 @@ void do_page_fault(struct cpu_user_regs *regs)
> 
>      if ( unlikely(current->domain->arch.suppress_spurious_page_faults) )
>      {
> -        pf_type = spurious_page_fault(addr, error_code);
> -        if ( pf_type == smep_fault )
> +        pf_type = spurious_page_fault(addr, regs);
> +        if ( (pf_type == smep_fault) || (pf_type == smap_fault))
>          {
> -            gdprintk(XENLOG_ERR, "Fatal SMEP fault\n");
> +            gdprintk(XENLOG_ERR, "Fatal %s fault\n",
> +                     (pf_type == smep_fault) ? "SMEP" : "SMAP");
>              domain_crash(current->domain);
>          }
>          if ( pf_type != real_fault )
> diff --git a/xen/include/asm-x86/cpufeature.h
> b/xen/include/asm-x86/cpufeature.h
> index 0c4d6c1..3ded6bd 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -186,6 +186,7 @@
>  #define cpu_has_fsgsbase	boot_cpu_has(X86_FEATURE_FSGSBASE)
> 
>  #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
> +#define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
>  #define cpu_has_fpu_sel
> (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
> 
>  #define cpu_has_ffxsr           ((boot_cpu_data.x86_vendor ==
> X86_VENDOR_AMD) \
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 4ff89f0..3b515f2 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -464,12 +464,14 @@ unsigned long pv_guest_cr4_fixup(const struct vcpu
> *, unsigned long guest_cr4);
>      (((v)->arch.pv_vcpu.ctrlreg[4]                          \
>        | (mmu_cr4_features                                   \
>           & (X86_CR4_PGE | X86_CR4_PSE | X86_CR4_SMEP |      \
> -            X86_CR4_OSXSAVE | X86_CR4_FSGSBASE))            \
> +            X86_CR4_SMAP | X86_CR4_OSXSAVE |                \
> +            X86_CR4_FSGSBASE))                              \
>        | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0))         \
>       & ~X86_CR4_DE)
>  #define real_cr4_to_pv_guest_cr4(c)                         \
>      ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD |      \
> -             X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE))
> +             X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
> +             X86_CR4_FSGSBASE | X86_CR4_SMAP))
> 
>  void domain_cpuid(struct domain *d,
>                    unsigned int  input,
> --
> 1.8.3.1

Thanks,
Feng

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Andrew Cooper]

On 15/04/14 14:01, Feng Wu wrote:
> Supervisor Mode Access Prevention (SMAP) is a new security
> feature disclosed by Intel, please refer to the following
> document:
>
> http://software.intel.com/sites/default/files/319433-014.pdf
>
> If CR4.SMAP = 1, supervisor-mode data accesses are not allowed
> to linear addresses that are accessible in user mode. If CPL < 3,
> SMAP protections are disabled if EFLAGS.AC = 1. If CPL = 3, SMAP
> applies to all supervisor-mode data accesses (these are implicit
> supervisor accesses) regardless of the value of EFLAGS.AC.
>
> This patch enables SMAP in Xen to prevent Xen hypervisor from
> accessing pv guest data, whose translation paging-structure
> entries' U/S flags are all set.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  xen/arch/x86/setup.c             |  9 +++++++++
>  xen/arch/x86/traps.c             | 34 ++++++++++++++++++++++++++--------
>  xen/include/asm-x86/cpufeature.h |  1 +
>  xen/include/asm-x86/domain.h     |  6 ++++--
>  4 files changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index e9c2c51..09c974d 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -61,6 +61,10 @@ integer_param("maxcpus", max_cpus);
>  static bool_t __initdata disable_smep;
>  invbool_param("smep", disable_smep);
>  
> +/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
> +static bool_t __initdata disable_smap;
> +invbool_param("smap", disable_smap);
> +

Please use a positive boolean rather than negative.  Convention is also
to prefix the variable with opt_

static bool_t __initdata opt_smap = 1;
boolean_param("smap", opt_smap);


Also, please patch docs/misc/xen-command-line.markdown

>  /* **** Linux config option: propagated to domain0. */
>  /* "acpi=off":    Sisables both ACPI table parsing and interpreter. */
>  /* "acpi=force":  Override the disable blacklist.                   */
> @@ -1279,6 +1283,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      if ( cpu_has_smep )
>          set_in_cr4(X86_CR4_SMEP);
>  
> +    if ( disable_smap )
> +        setup_clear_cpu_cap(X86_FEATURE_SMAP);
> +    if ( cpu_has_smap )
> +        set_in_cr4(X86_CR4_SMAP);
> +
>      if ( cpu_has_fsgsbase )
>          set_in_cr4(X86_CR4_FSGSBASE);
>  
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index ed4ae2d..9307f10 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1182,11 +1182,12 @@ static int handle_gdt_ldt_mapping_fault(
>  enum pf_type {
>      real_fault,
>      smep_fault,
> +    smap_fault,
>      spurious_fault
>  };
>  
>  static enum pf_type __page_fault_type(
> -    unsigned long addr, unsigned int error_code)
> +    unsigned long addr, struct cpu_user_regs *regs)
>  {
>      unsigned long mfn, cr3 = read_cr3();
>      l4_pgentry_t l4e, *l4t;
> @@ -1194,6 +1195,7 @@ static enum pf_type __page_fault_type(
>      l2_pgentry_t l2e, *l2t;
>      l1_pgentry_t l1e, *l1t;
>      unsigned int required_flags, disallowed_flags, page_user;
> +    unsigned int error_code = regs->error_code;
>  
>      /*
>       * We do not take spurious page faults in IRQ handlers as we do not
> @@ -1270,11 +1272,26 @@ leaf:
>           ((error_code & (PFEC_insn_fetch|PFEC_user_mode)) == PFEC_insn_fetch) )
>          return smep_fault;
>  
> +    /*
> +     * Supervisor Mode Access Prevention (SMAP):
> +     * Disallow supervisor access user-accessible mappings
> +     * A fault is considered as an SMAP violation if the following
> +     * conditions are ture:

'true'

> +     *   - X86_CR4_SMAP is set in CR4
> +     *   - An user page is accessed
> +     *   - CPL=3 or X86_EFLAGS_AC is clear

CPL < 3 surely?

~Andrew

> +     *   - Page fault in kernel mode
> +     */
> +    if ( (read_cr4() & X86_CR4_SMAP) && page_user &&
> +         !(((regs->cs & 0x03) < 3) && (regs->eflags & X86_EFLAGS_AC)) &&
> +         !(error_code & PFEC_user_mode) )
> +        return smap_fault;
> +
>      return spurious_fault;
>  }
>  
>  static enum pf_type spurious_page_fault(
> -    unsigned long addr, unsigned int error_code)
> +    unsigned long addr, struct cpu_user_regs *regs)
>  {
>      unsigned long flags;
>      enum pf_type pf_type;
> @@ -1284,7 +1301,7 @@ static enum pf_type spurious_page_fault(
>       * page tables from becoming invalid under our feet during the walk.
>       */
>      local_irq_save(flags);
> -    pf_type = __page_fault_type(addr, error_code);
> +    pf_type = __page_fault_type(addr, regs);
>      local_irq_restore(flags);
>  
>      return pf_type;
> @@ -1379,8 +1396,8 @@ void do_page_fault(struct cpu_user_regs *regs)
>  
>      if ( unlikely(!guest_mode(regs)) )
>      {
> -        pf_type = spurious_page_fault(addr, error_code);
> -        BUG_ON(pf_type == smep_fault);
> +        pf_type = spurious_page_fault(addr, regs);
> +        BUG_ON((pf_type == smep_fault) || (pf_type == smap_fault));
>          if ( pf_type != real_fault )
>              return;
>  
> @@ -1406,10 +1423,11 @@ void do_page_fault(struct cpu_user_regs *regs)
>  
>      if ( unlikely(current->domain->arch.suppress_spurious_page_faults) )
>      {
> -        pf_type = spurious_page_fault(addr, error_code);
> -        if ( pf_type == smep_fault )
> +        pf_type = spurious_page_fault(addr, regs);
> +        if ( (pf_type == smep_fault) || (pf_type == smap_fault))
>          {
> -            gdprintk(XENLOG_ERR, "Fatal SMEP fault\n");
> +            gdprintk(XENLOG_ERR, "Fatal %s fault\n",
> +                     (pf_type == smep_fault) ? "SMEP" : "SMAP");
>              domain_crash(current->domain);
>          }
>          if ( pf_type != real_fault )
> diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
> index 0c4d6c1..3ded6bd 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -186,6 +186,7 @@
>  #define cpu_has_fsgsbase	boot_cpu_has(X86_FEATURE_FSGSBASE)
>  
>  #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
> +#define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
>  #define cpu_has_fpu_sel         (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
>  
>  #define cpu_has_ffxsr           ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) \
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 4ff89f0..3b515f2 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -464,12 +464,14 @@ unsigned long pv_guest_cr4_fixup(const struct vcpu *, unsigned long guest_cr4);
>      (((v)->arch.pv_vcpu.ctrlreg[4]                          \
>        | (mmu_cr4_features                                   \
>           & (X86_CR4_PGE | X86_CR4_PSE | X86_CR4_SMEP |      \
> -            X86_CR4_OSXSAVE | X86_CR4_FSGSBASE))            \
> +            X86_CR4_SMAP | X86_CR4_OSXSAVE |                \
> +            X86_CR4_FSGSBASE))                              \
>        | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0))         \
>       & ~X86_CR4_DE)
>  #define real_cr4_to_pv_guest_cr4(c)                         \
>      ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD |      \
> -             X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE))
> +             X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
> +             X86_CR4_FSGSBASE | X86_CR4_SMAP))
>  
>  void domain_cpuid(struct domain *d,
>                    unsigned int  input,

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Jan Beulich]

>>> On 15.04.14 at 12:32, <andrew.cooper3@citrix.com> wrote:
> On 15/04/14 14:01, Feng Wu wrote:
>> index e9c2c51..09c974d 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -61,6 +61,10 @@ integer_param("maxcpus", max_cpus);
>>  static bool_t __initdata disable_smep;
>>  invbool_param("smep", disable_smep);
>>  
>> +/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
>> +static bool_t __initdata disable_smap;
>> +invbool_param("smap", disable_smap);
>> +
> 
> Please use a positive boolean rather than negative.  Convention is also
> to prefix the variable with opt_
> 
> static bool_t __initdata opt_smap = 1;
> boolean_param("smap", opt_smap);

Hmm, I'd go for consistency with SMAP here as a first step.
Converting both may later be an option, but I'm not really sure
why you think the invbool_param() is bad.

Jan

[PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Feng Wu]

Supervisor Mode Access Prevention (SMAP) is a new security
feature disclosed by Intel, please refer to the following
document:

http://software.intel.com/sites/default/files/319433-014.pdf

If CR4.SMAP = 1, supervisor-mode data accesses are not allowed
to linear addresses that are accessible in user mode. If CPL < 3,
SMAP protections are disabled if EFLAGS.AC = 1. If CPL = 3, SMAP
applies to all supervisor-mode data accesses (these are implicit
supervisor accesses) regardless of the value of EFLAGS.AC.

This patch enables SMAP in Xen to prevent Xen hypervisor from
accessing pv guest data, whose translation paging-structure
entries' U/S flags are all set.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 xen/arch/x86/setup.c             |  9 +++++++++
 xen/arch/x86/traps.c             | 34 ++++++++++++++++++++++++++--------
 xen/include/asm-x86/cpufeature.h |  1 +
 xen/include/asm-x86/domain.h     |  6 ++++--
 4 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index e9c2c51..09c974d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -61,6 +61,10 @@ integer_param("maxcpus", max_cpus);
 static bool_t __initdata disable_smep;
 invbool_param("smep", disable_smep);
 
+/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
+static bool_t __initdata disable_smap;
+invbool_param("smap", disable_smap);
+
 /* **** Linux config option: propagated to domain0. */
 /* "acpi=off":    Sisables both ACPI table parsing and interpreter. */
 /* "acpi=force":  Override the disable blacklist.                   */
@@ -1279,6 +1283,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     if ( cpu_has_smep )
         set_in_cr4(X86_CR4_SMEP);
 
+    if ( disable_smap )
+        setup_clear_cpu_cap(X86_FEATURE_SMAP);
+    if ( cpu_has_smap )
+        set_in_cr4(X86_CR4_SMAP);
+
     if ( cpu_has_fsgsbase )
         set_in_cr4(X86_CR4_FSGSBASE);
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index ed4ae2d..9307f10 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1182,11 +1182,12 @@ static int handle_gdt_ldt_mapping_fault(
 enum pf_type {
     real_fault,
     smep_fault,
+    smap_fault,
     spurious_fault
 };
 
 static enum pf_type __page_fault_type(
-    unsigned long addr, unsigned int error_code)
+    unsigned long addr, struct cpu_user_regs *regs)
 {
     unsigned long mfn, cr3 = read_cr3();
     l4_pgentry_t l4e, *l4t;
@@ -1194,6 +1195,7 @@ static enum pf_type __page_fault_type(
     l2_pgentry_t l2e, *l2t;
     l1_pgentry_t l1e, *l1t;
     unsigned int required_flags, disallowed_flags, page_user;
+    unsigned int error_code = regs->error_code;
 
     /*
      * We do not take spurious page faults in IRQ handlers as we do not
@@ -1270,11 +1272,26 @@ leaf:
          ((error_code & (PFEC_insn_fetch|PFEC_user_mode)) == PFEC_insn_fetch) )
         return smep_fault;
 
+    /*
+     * Supervisor Mode Access Prevention (SMAP):
+     * Disallow supervisor access user-accessible mappings
+     * A fault is considered as an SMAP violation if the following
+     * conditions are ture:
+     *   - X86_CR4_SMAP is set in CR4
+     *   - An user page is accessed
+     *   - CPL=3 or X86_EFLAGS_AC is clear
+     *   - Page fault in kernel mode
+     */
+    if ( (read_cr4() & X86_CR4_SMAP) && page_user &&
+         !(((regs->cs & 0x03) < 3) && (regs->eflags & X86_EFLAGS_AC)) &&
+         !(error_code & PFEC_user_mode) )
+        return smap_fault;
+
     return spurious_fault;
 }
 
 static enum pf_type spurious_page_fault(
-    unsigned long addr, unsigned int error_code)
+    unsigned long addr, struct cpu_user_regs *regs)
 {
     unsigned long flags;
     enum pf_type pf_type;
@@ -1284,7 +1301,7 @@ static enum pf_type spurious_page_fault(
      * page tables from becoming invalid under our feet during the walk.
      */
     local_irq_save(flags);
-    pf_type = __page_fault_type(addr, error_code);
+    pf_type = __page_fault_type(addr, regs);
     local_irq_restore(flags);
 
     return pf_type;
@@ -1379,8 +1396,8 @@ void do_page_fault(struct cpu_user_regs *regs)
 
     if ( unlikely(!guest_mode(regs)) )
     {
-        pf_type = spurious_page_fault(addr, error_code);
-        BUG_ON(pf_type == smep_fault);
+        pf_type = spurious_page_fault(addr, regs);
+        BUG_ON((pf_type == smep_fault) || (pf_type == smap_fault));
         if ( pf_type != real_fault )
             return;
 
@@ -1406,10 +1423,11 @@ void do_page_fault(struct cpu_user_regs *regs)
 
     if ( unlikely(current->domain->arch.suppress_spurious_page_faults) )
     {
-        pf_type = spurious_page_fault(addr, error_code);
-        if ( pf_type == smep_fault )
+        pf_type = spurious_page_fault(addr, regs);
+        if ( (pf_type == smep_fault) || (pf_type == smap_fault))
         {
-            gdprintk(XENLOG_ERR, "Fatal SMEP fault\n");
+            gdprintk(XENLOG_ERR, "Fatal %s fault\n",
+                     (pf_type == smep_fault) ? "SMEP" : "SMAP");
             domain_crash(current->domain);
         }
         if ( pf_type != real_fault )
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 0c4d6c1..3ded6bd 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -186,6 +186,7 @@
 #define cpu_has_fsgsbase	boot_cpu_has(X86_FEATURE_FSGSBASE)
 
 #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
+#define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
 #define cpu_has_fpu_sel         (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
 
 #define cpu_has_ffxsr           ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) \
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4ff89f0..3b515f2 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -464,12 +464,14 @@ unsigned long pv_guest_cr4_fixup(const struct vcpu *, unsigned long guest_cr4);
     (((v)->arch.pv_vcpu.ctrlreg[4]                          \
       | (mmu_cr4_features                                   \
          & (X86_CR4_PGE | X86_CR4_PSE | X86_CR4_SMEP |      \
-            X86_CR4_OSXSAVE | X86_CR4_FSGSBASE))            \
+            X86_CR4_SMAP | X86_CR4_OSXSAVE |                \
+            X86_CR4_FSGSBASE))                              \
       | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0))         \
      & ~X86_CR4_DE)
 #define real_cr4_to_pv_guest_cr4(c)                         \
     ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD |      \
-             X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE))
+             X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
+             X86_CR4_FSGSBASE | X86_CR4_SMAP))
 
 void domain_cpuid(struct domain *d,
                   unsigned int  input,
-- 
1.8.3.1

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Andrew Cooper]

On 15/04/14 12:48, Jan Beulich wrote:
>>>> On 15.04.14 at 12:32, <andrew.cooper3@citrix.com> wrote:
>> On 15/04/14 14:01, Feng Wu wrote:
>>> index e9c2c51..09c974d 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -61,6 +61,10 @@ integer_param("maxcpus", max_cpus);
>>>  static bool_t __initdata disable_smep;
>>>  invbool_param("smep", disable_smep);
>>>  
>>> +/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
>>> +static bool_t __initdata disable_smap;
>>> +invbool_param("smap", disable_smap);
>>> +
>> Please use a positive boolean rather than negative.  Convention is also
>> to prefix the variable with opt_
>>
>> static bool_t __initdata opt_smap = 1;
>> boolean_param("smap", opt_smap);
> Hmm, I'd go for consistency with SMAP here as a first step.
> Converting both may later be an option, but I'm not really sure
> why you think the invbool_param() is bad.
>
> Jan
>

Not invbool_param() per say, but with negative booleans in general.  It
is just unnecessary extra cognitive load when following code.  This is
admittedly a mild example.

~Andrew

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Andrew Cooper]

On 15/04/14 14:01, Feng Wu wrote:
> @@ -1379,8 +1396,8 @@ void do_page_fault(struct cpu_user_regs *regs)
>  
>      if ( unlikely(!guest_mode(regs)) )
>      {
> -        pf_type = spurious_page_fault(addr, error_code);
> -        BUG_ON(pf_type == smep_fault);
> +        pf_type = spurious_page_fault(addr, regs);
> +        BUG_ON((pf_type == smep_fault) || (pf_type == smap_fault));

On further consideration, given the nature of faults like these, this
code would be better as:

if ( (pf_type == smep_fault) || (pf_type == smap_fault) )
{
    console_start_sync();
    printk("Xen SM%cP violation", pf_type == smep_fault ? 'E' : 'A');
    fatal_trap(TRAP_page_fault, regs);
}

This would make the resulting crash crystal clear as to what went wrong,
and forgo the first step of having to look up which BUG() tripped.

~Andrew

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Andrew Cooper]

On 15/04/14 15:00, Andrew Cooper wrote:
> On 15/04/14 14:01, Feng Wu wrote:
>> @@ -1379,8 +1396,8 @@ void do_page_fault(struct cpu_user_regs *regs)
>>  
>>      if ( unlikely(!guest_mode(regs)) )
>>      {
>> -        pf_type = spurious_page_fault(addr, error_code);
>> -        BUG_ON(pf_type == smep_fault);
>> +        pf_type = spurious_page_fault(addr, regs);
>> +        BUG_ON((pf_type == smep_fault) || (pf_type == smap_fault));
> On further consideration, given the nature of faults like these, this
> code would be better as:
>
> if ( (pf_type == smep_fault) || (pf_type == smap_fault) )
> {
>     console_start_sync();
>     printk("Xen SM%cP violation", pf_type == smep_fault ? 'E' : 'A');
>     fatal_trap(TRAP_page_fault, regs);
> }
>
> This would make the resulting crash crystal clear as to what went wrong,
> and forgo the first step of having to look up which BUG() tripped.
>
> ~Andrew

And having just sent this email, I further realise that functions like
show_page_walk() need protection against SMAP otherwise we will take a
recursive fault when trying to dump the error information from the first
fault.  I don't recall any of your other patches dealing with this.

All this error printing can probably better be colated together after
the exception table search.

~Andrew

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Jan Beulich]

>>> On 15.04.14 at 16:09, <andrew.cooper3@citrix.com> wrote:
> And having just sent this email, I further realise that functions like
> show_page_walk() need protection against SMAP otherwise we will take a
> recursive fault when trying to dump the error information from the first
> fault.  I don't recall any of your other patches dealing with this.

I don't follow: page table walks are done using map_domain_page(),
which ought to not produce user mode accessible mappings. Or did
you mean to say "may" instead of "will", and meant to do this just to
be on the safe side?

Jan

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Andrew Cooper]

On 15/04/14 15:16, Jan Beulich wrote:
>>>> On 15.04.14 at 16:09, <andrew.cooper3@citrix.com> wrote:
>> And having just sent this email, I further realise that functions like
>> show_page_walk() need protection against SMAP otherwise we will take a
>> recursive fault when trying to dump the error information from the first
>> fault.  I don't recall any of your other patches dealing with this.
> I don't follow: page table walks are done using map_domain_page(),
> which ought to not produce user mode accessible mappings. Or did
> you mean to say "may" instead of "will", and meant to do this just to
> be on the safe side?
>
> Jan
>

Hmm yes - in retrospect we shouldn't actually take faults from domain
mapped pages.

However for the safe side of things, we don't want to be taking
recursive faults in a panic scenario, so it might be a good idea to
preemptively stac() on terminal error paths.

~Andrew

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Wu, Feng]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, April 15, 2014 6:33 PM
> To: Wu, Feng
> Cc: JBeulich@suse.com; Ian.Campbell@citrix.com; xen-devel@lists.xen.org;
> Dong, Eddie; Nakajima, Jun
> Subject: Re: [Xen-devel] [PATCH v1 3/6] x86: Enable Supervisor Mode Execution
> Prevention (SMAP) for Xen
> 
> On 15/04/14 14:01, Feng Wu wrote:
> > Supervisor Mode Access Prevention (SMAP) is a new security
> > feature disclosed by Intel, please refer to the following
> > document:
> >
> > http://software.intel.com/sites/default/files/319433-014.pdf
> >
> > If CR4.SMAP = 1, supervisor-mode data accesses are not allowed
> > to linear addresses that are accessible in user mode. If CPL < 3,
> > SMAP protections are disabled if EFLAGS.AC = 1. If CPL = 3, SMAP
> > applies to all supervisor-mode data accesses (these are implicit
> > supervisor accesses) regardless of the value of EFLAGS.AC.
> >
> > This patch enables SMAP in Xen to prevent Xen hypervisor from
> > accessing pv guest data, whose translation paging-structure
> > entries' U/S flags are all set.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> >  xen/arch/x86/setup.c             |  9 +++++++++
> >  xen/arch/x86/traps.c             | 34
> ++++++++++++++++++++++++++--------
> >  xen/include/asm-x86/cpufeature.h |  1 +
> >  xen/include/asm-x86/domain.h     |  6 ++++--
> >  4 files changed, 40 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index e9c2c51..09c974d 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -61,6 +61,10 @@ integer_param("maxcpus", max_cpus);
> >  static bool_t __initdata disable_smep;
> >  invbool_param("smep", disable_smep);
> >
> > +/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
> > +static bool_t __initdata disable_smap;
> > +invbool_param("smap", disable_smap);
> > +
> 
> Please use a positive boolean rather than negative.  Convention is also
> to prefix the variable with opt_
> 
> static bool_t __initdata opt_smap = 1;
> boolean_param("smap", opt_smap);
> 

Since SMAP is a similar feature compared to SMEP, here I just follow the
existing code of SMEP.

> 
> Also, please patch docs/misc/xen-command-line.markdown
> 
> >  /* **** Linux config option: propagated to domain0. */
> >  /* "acpi=off":    Sisables both ACPI table parsing and interpreter. */
> >  /* "acpi=force":  Override the disable blacklist.                   */
> > @@ -1279,6 +1283,11 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
> >      if ( cpu_has_smep )
> >          set_in_cr4(X86_CR4_SMEP);
> >
> > +    if ( disable_smap )
> > +        setup_clear_cpu_cap(X86_FEATURE_SMAP);
> > +    if ( cpu_has_smap )
> > +        set_in_cr4(X86_CR4_SMAP);
> > +
> >      if ( cpu_has_fsgsbase )
> >          set_in_cr4(X86_CR4_FSGSBASE);
> >
> > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > index ed4ae2d..9307f10 100644
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -1182,11 +1182,12 @@ static int handle_gdt_ldt_mapping_fault(
> >  enum pf_type {
> >      real_fault,
> >      smep_fault,
> > +    smap_fault,
> >      spurious_fault
> >  };
> >
> >  static enum pf_type __page_fault_type(
> > -    unsigned long addr, unsigned int error_code)
> > +    unsigned long addr, struct cpu_user_regs *regs)
> >  {
> >      unsigned long mfn, cr3 = read_cr3();
> >      l4_pgentry_t l4e, *l4t;
> > @@ -1194,6 +1195,7 @@ static enum pf_type __page_fault_type(
> >      l2_pgentry_t l2e, *l2t;
> >      l1_pgentry_t l1e, *l1t;
> >      unsigned int required_flags, disallowed_flags, page_user;
> > +    unsigned int error_code = regs->error_code;
> >
> >      /*
> >       * We do not take spurious page faults in IRQ handlers as we do not
> > @@ -1270,11 +1272,26 @@ leaf:
> >           ((error_code & (PFEC_insn_fetch|PFEC_user_mode)) ==
> PFEC_insn_fetch) )
> >          return smep_fault;
> >
> > +    /*
> > +     * Supervisor Mode Access Prevention (SMAP):
> > +     * Disallow supervisor access user-accessible mappings
> > +     * A fault is considered as an SMAP violation if the following
> > +     * conditions are ture:
> 
> 'true'
> 
> > +     *   - X86_CR4_SMAP is set in CR4
> > +     *   - An user page is accessed
> > +     *   - CPL=3 or X86_EFLAGS_AC is clear
> 
> CPL < 3 surely?
> 
> ~Andrew

The comments here is correct. SMAP takes effect when:
	- CPL == 3
	or
	- CPL <3 && AC bit is clear

> 
> > +     *   - Page fault in kernel mode
> > +     */
> > +    if ( (read_cr4() & X86_CR4_SMAP) && page_user &&
> > +         !(((regs->cs & 0x03) < 3) && (regs->eflags & X86_EFLAGS_AC))
> &&
> > +         !(error_code & PFEC_user_mode) )
> > +        return smap_fault;
> > +
> >      return spurious_fault;
> >  }
> >
> >  static enum pf_type spurious_page_fault(
> > -    unsigned long addr, unsigned int error_code)
> > +    unsigned long addr, struct cpu_user_regs *regs)
> >  {
> >      unsigned long flags;
> >      enum pf_type pf_type;
> > @@ -1284,7 +1301,7 @@ static enum pf_type spurious_page_fault(
> >       * page tables from becoming invalid under our feet during the walk.
> >       */
> >      local_irq_save(flags);
> > -    pf_type = __page_fault_type(addr, error_code);
> > +    pf_type = __page_fault_type(addr, regs);
> >      local_irq_restore(flags);
> >
> >      return pf_type;
> > @@ -1379,8 +1396,8 @@ void do_page_fault(struct cpu_user_regs *regs)
> >
> >      if ( unlikely(!guest_mode(regs)) )
> >      {
> > -        pf_type = spurious_page_fault(addr, error_code);
> > -        BUG_ON(pf_type == smep_fault);
> > +        pf_type = spurious_page_fault(addr, regs);
> > +        BUG_ON((pf_type == smep_fault) || (pf_type == smap_fault));
> >          if ( pf_type != real_fault )
> >              return;
> >
> > @@ -1406,10 +1423,11 @@ void do_page_fault(struct cpu_user_regs *regs)
> >
> >      if ( unlikely(current->domain->arch.suppress_spurious_page_faults) )
> >      {
> > -        pf_type = spurious_page_fault(addr, error_code);
> > -        if ( pf_type == smep_fault )
> > +        pf_type = spurious_page_fault(addr, regs);
> > +        if ( (pf_type == smep_fault) || (pf_type == smap_fault))
> >          {
> > -            gdprintk(XENLOG_ERR, "Fatal SMEP fault\n");
> > +            gdprintk(XENLOG_ERR, "Fatal %s fault\n",
> > +                     (pf_type == smep_fault) ? "SMEP" : "SMAP");
> >              domain_crash(current->domain);
> >          }
> >          if ( pf_type != real_fault )
> > diff --git a/xen/include/asm-x86/cpufeature.h
> b/xen/include/asm-x86/cpufeature.h
> > index 0c4d6c1..3ded6bd 100644
> > --- a/xen/include/asm-x86/cpufeature.h
> > +++ b/xen/include/asm-x86/cpufeature.h
> > @@ -186,6 +186,7 @@
> >  #define cpu_has_fsgsbase	boot_cpu_has(X86_FEATURE_FSGSBASE)
> >
> >  #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
> > +#define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
> >  #define cpu_has_fpu_sel
> (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
> >
> >  #define cpu_has_ffxsr           ((boot_cpu_data.x86_vendor ==
> X86_VENDOR_AMD) \
> > diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> > index 4ff89f0..3b515f2 100644
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -464,12 +464,14 @@ unsigned long pv_guest_cr4_fixup(const struct
> vcpu *, unsigned long guest_cr4);
> >      (((v)->arch.pv_vcpu.ctrlreg[4]                          \
> >        | (mmu_cr4_features                                   \
> >           & (X86_CR4_PGE | X86_CR4_PSE | X86_CR4_SMEP |      \
> > -            X86_CR4_OSXSAVE | X86_CR4_FSGSBASE))            \
> > +            X86_CR4_SMAP | X86_CR4_OSXSAVE |                \
> > +            X86_CR4_FSGSBASE))                              \
> >        | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0))         \
> >       & ~X86_CR4_DE)
> >  #define real_cr4_to_pv_guest_cr4(c)                         \
> >      ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD |      \
> > -             X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE))
> > +             X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
> > +             X86_CR4_FSGSBASE | X86_CR4_SMAP))
> >
> >  void domain_cpuid(struct domain *d,
> >                    unsigned int  input,

Thanks,
Feng

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Andrew Cooper]

On 16/04/14 03:20, Wu, Feng wrote:
>
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: Tuesday, April 15, 2014 6:33 PM
>> To: Wu, Feng
>> Cc: JBeulich@suse.com; Ian.Campbell@citrix.com; xen-devel@lists.xen.org;
>> Dong, Eddie; Nakajima, Jun
>> Subject: Re: [Xen-devel] [PATCH v1 3/6] x86: Enable Supervisor Mode Execution
>> Prevention (SMAP) for Xen
>>
>> On 15/04/14 14:01, Feng Wu wrote:
>>> Supervisor Mode Access Prevention (SMAP) is a new security
>>> feature disclosed by Intel, please refer to the following
>>> document:
>>>
>>> http://software.intel.com/sites/default/files/319433-014.pdf
>>>
>>> If CR4.SMAP = 1, supervisor-mode data accesses are not allowed
>>> to linear addresses that are accessible in user mode. If CPL < 3,
>>> SMAP protections are disabled if EFLAGS.AC = 1. If CPL = 3, SMAP
>>> applies to all supervisor-mode data accesses (these are implicit
>>> supervisor accesses) regardless of the value of EFLAGS.AC.
>>>
>>> This patch enables SMAP in Xen to prevent Xen hypervisor from
>>> accessing pv guest data, whose translation paging-structure
>>> entries' U/S flags are all set.
>>>
>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>> ---
>>>  xen/arch/x86/setup.c             |  9 +++++++++
>>>  xen/arch/x86/traps.c             | 34
>> ++++++++++++++++++++++++++--------
>>>  xen/include/asm-x86/cpufeature.h |  1 +
>>>  xen/include/asm-x86/domain.h     |  6 ++++--
>>>  4 files changed, 40 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index e9c2c51..09c974d 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -61,6 +61,10 @@ integer_param("maxcpus", max_cpus);
>>>  static bool_t __initdata disable_smep;
>>>  invbool_param("smep", disable_smep);
>>>
>>> +/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
>>> +static bool_t __initdata disable_smap;
>>> +invbool_param("smap", disable_smap);
>>> +
>> Please use a positive boolean rather than negative.  Convention is also
>> to prefix the variable with opt_
>>
>> static bool_t __initdata opt_smap = 1;
>> boolean_param("smap", opt_smap);
>>
> Since SMAP is a similar feature compared to SMEP, here I just follow the
> existing code of SMEP.
>
>> Also, please patch docs/misc/xen-command-line.markdown
>>
>>>  /* **** Linux config option: propagated to domain0. */
>>>  /* "acpi=off":    Sisables both ACPI table parsing and interpreter. */
>>>  /* "acpi=force":  Override the disable blacklist.                   */
>>> @@ -1279,6 +1283,11 @@ void __init noreturn __start_xen(unsigned long
>> mbi_p)
>>>      if ( cpu_has_smep )
>>>          set_in_cr4(X86_CR4_SMEP);
>>>
>>> +    if ( disable_smap )
>>> +        setup_clear_cpu_cap(X86_FEATURE_SMAP);
>>> +    if ( cpu_has_smap )
>>> +        set_in_cr4(X86_CR4_SMAP);
>>> +
>>>      if ( cpu_has_fsgsbase )
>>>          set_in_cr4(X86_CR4_FSGSBASE);
>>>
>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>>> index ed4ae2d..9307f10 100644
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -1182,11 +1182,12 @@ static int handle_gdt_ldt_mapping_fault(
>>>  enum pf_type {
>>>      real_fault,
>>>      smep_fault,
>>> +    smap_fault,
>>>      spurious_fault
>>>  };
>>>
>>>  static enum pf_type __page_fault_type(
>>> -    unsigned long addr, unsigned int error_code)
>>> +    unsigned long addr, struct cpu_user_regs *regs)
>>>  {
>>>      unsigned long mfn, cr3 = read_cr3();
>>>      l4_pgentry_t l4e, *l4t;
>>> @@ -1194,6 +1195,7 @@ static enum pf_type __page_fault_type(
>>>      l2_pgentry_t l2e, *l2t;
>>>      l1_pgentry_t l1e, *l1t;
>>>      unsigned int required_flags, disallowed_flags, page_user;
>>> +    unsigned int error_code = regs->error_code;
>>>
>>>      /*
>>>       * We do not take spurious page faults in IRQ handlers as we do not
>>> @@ -1270,11 +1272,26 @@ leaf:
>>>           ((error_code & (PFEC_insn_fetch|PFEC_user_mode)) ==
>> PFEC_insn_fetch) )
>>>          return smep_fault;
>>>
>>> +    /*
>>> +     * Supervisor Mode Access Prevention (SMAP):
>>> +     * Disallow supervisor access user-accessible mappings
>>> +     * A fault is considered as an SMAP violation if the following
>>> +     * conditions are ture:
>> 'true'
>>
>>> +     *   - X86_CR4_SMAP is set in CR4
>>> +     *   - An user page is accessed
>>> +     *   - CPL=3 or X86_EFLAGS_AC is clear
>> CPL < 3 surely?
>>
>> ~Andrew
> The comments here is correct. SMAP takes effect when:
> 	- CPL == 3
> 	or
> 	- CPL <3 && AC bit is clear
>
>

Which is even further from your written comment, although given the
bullet points in the manual I can see why you expressed it this way.

On a broader note, how do implicit supervisor accesses interact with
SMAP when the usermode code is using alignment checking?  Do we end up
with spurious SMAP violations for accesses to the GDT, LDT, IDT and TSS?

~Andrew

Re: [PATCH v1 3/6] x86: Enable Supervisor Mode Execution Prevention (SMAP) for Xen

[Jan Beulich]

>>> On 16.04.14 at 11:10, <andrew.cooper3@citrix.com> wrote:
> On a broader note, how do implicit supervisor accesses interact with
> SMAP when the usermode code is using alignment checking?  Do we end up
> with spurious SMAP violations for accesses to the GDT, LDT, IDT and TSS?

None of these should be mapped with the U bit set at all levels.

Jan