Re: [PATCH v7 5/7] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/05/14 10:21, 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>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  docs/misc/xen-command-line.markdown |  7 +++++
>  xen/arch/x86/setup.c                | 20 +++++++++++++
>  xen/arch/x86/traps.c                | 58 +++++++++++++++++++++++++++----------
>  xen/include/asm-x86/cpufeature.h    |  1 +
>  xen/include/asm-x86/domain.h        |  6 ++--
>  5 files changed, 75 insertions(+), 17 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 7dc938b..a7ac53d 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -912,6 +912,13 @@ Set the serial transmit buffer size.
>  
>  Flag to enable Supervisor Mode Execution Protection
>  
> +### smap
> +> `= <boolean>`
> +
> +> Default: `true`
> +
> +Flag to enable Supervisor Mode Access Prevention
> +
>  ### snb\_igd\_quirk
>  > `= <boolean>`
>  
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 2e30701..d8598a3 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.                   */
> @@ -1280,6 +1284,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);
>  
> @@ -1386,6 +1395,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                 initrdidx);
>  
>      /*
> +     * Temporarily clear SMAP in CR4 to allow user-accesses in construct_dom0().
> +     * This saves a large number of corner cases interactions with
> +     * copy_from_user().
> +     */
> +    if ( cpu_has_smap )
> +        write_cr4(read_cr4() & ~X86_CR4_SMAP);
> +
> +    /*
>       * We're going to setup domain0 using the module(s) that we stashed safely
>       * above our heap. The second module, if present, is an initrd ramdisk.
>       */
> @@ -1395,6 +1412,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                          bootstrap_map, cmdline) != 0)
>          panic("Could not set up DOM0 guest OS");
>  
> +    if ( cpu_has_smap )
> +        write_cr4(read_cr4() | X86_CR4_SMAP);
> +
>      /* Scrub RAM that is still free and so may go to an unprivileged domain. */
>      scrub_heap_pages();
>  
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index c992df1..44b92af 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1197,11 +1197,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, const struct cpu_user_regs *regs)
>  {
>      unsigned long mfn, cr3 = read_cr3();
>      l4_pgentry_t l4e, *l4t;
> @@ -1209,6 +1210,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
> @@ -1277,19 +1279,37 @@ static enum pf_type __page_fault_type(
>      page_user &= l1e_get_flags(l1e);
>  
>  leaf:
> -    /*
> -     * Supervisor Mode Execution Protection (SMEP):
> -     * Disallow supervisor execution from user-accessible mappings
> -     */
> -    if ( (read_cr4() & X86_CR4_SMEP) && page_user &&
> -         ((error_code & (PFEC_insn_fetch|PFEC_user_mode)) == PFEC_insn_fetch) )
> -        return smep_fault;
> +    if ( page_user )
> +    {
> +        unsigned long cr4 = read_cr4();
> +        /*
> +         * Supervisor Mode Execution Prevention (SMEP):
> +         * Disallow supervisor execution from user-accessible mappings
> +         */
> +        if ( (cr4 & X86_CR4_SMEP) &&
> +             ((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 true:
> +         *   - X86_CR4_SMAP is set in CR4
> +         *   - A user page is being accessed
> +         *   - CPL=3 or X86_EFLAGS_AC is clear
> +         *   - Page fault in kernel mode
> +         */
> +        if ( (cr4 & X86_CR4_SMAP) && !(error_code & PFEC_user_mode) &&
> +             (((regs->cs & 3) == 3) || !(regs->eflags & X86_EFLAGS_AC)) )
> +            return smap_fault;
> +    }
>  
>      return spurious_fault;
>  }
>  
>  static enum pf_type spurious_page_fault(
> -    unsigned long addr, unsigned int error_code)
> +    unsigned long addr, const struct cpu_user_regs *regs)
>  {
>      unsigned long flags;
>      enum pf_type pf_type;
> @@ -1299,7 +1319,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;
> @@ -1394,8 +1414,14 @@ 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);
> +        if ( (pf_type == smep_fault) || (pf_type == smap_fault) )
> +        {
> +            console_start_sync();
> +            printk("Xen SM%cP violation\n", (pf_type == smep_fault) ? 'E' : 'A');
> +            fatal_trap(TRAP_page_fault, regs);
> +        }
> +
>          if ( pf_type != real_fault )
>              return;
>  
> @@ -1421,10 +1447,12 @@ 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");
> +            printk(XENLOG_G_ERR "%pv fatal SM%cP violation\n",
> +                   current, (pf_type == smep_fault) ? 'E' : 'A');
> +
>              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 20881c0..8014241 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -190,6 +190,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 c5c266f..abf55fb 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -467,12 +467,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,