[PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

[PATCH v4 08/10] x86: Enable Supervisor Mode Access 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>
---
 docs/misc/xen-command-line.markdown |  7 +++++
 xen/arch/x86/setup.c                | 14 +++++++++
 xen/arch/x86/traps.c                | 58 +++++++++++++++++++++++++++----------
 xen/include/asm-x86/cpufeature.h    |  1 +
 xen/include/asm-x86/domain.h        |  6 ++--
 5 files changed, 69 insertions(+), 17 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 689ffe6..b9b38ad 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -860,6 +860,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 e9c2c51..be98f79 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,9 @@ 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_fsgsbase )
         set_in_cr4(X86_CR4_FSGSBASE);
 
@@ -1394,6 +1401,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                         bootstrap_map, cmdline) != 0)
         panic("Could not set up DOM0 guest OS");
 
+    /*
+     * Enable SMAP after constructing domain0, since there are lots of accesses to
+     * user pages in construct_dom0(), which is safe at the current stage.
+     */
+    if ( cpu_has_smap )
+        set_in_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 98044a3..8d58592 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, const 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
@@ -1262,19 +1264,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, struct cpu_user_regs *regs)
 {
     unsigned long flags;
     enum pf_type pf_type;
@@ -1284,7 +1304,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 +1399,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;
 
@@ -1406,10 +1432,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 3dfb875..9a5b18d 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -189,6 +189,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 v4 08/10] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

[Jan Beulich]

>>> On 05.05.14 at 10:23, <feng.wu@intel.com> wrote:
> @@ -1394,6 +1401,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                          bootstrap_map, cmdline) != 0)
>          panic("Could not set up DOM0 guest OS");
>  
> +    /*
> +     * Enable SMAP after constructing domain0, since there are lots of accesses to
> +     * user pages in construct_dom0(), which is safe at the current stage.
> +     */
> +    if ( cpu_has_smap )
> +        set_in_cr4(X86_CR4_SMAP);
> +

I should have noticed this on the previous round already - the way
it's being done right now will leave APs with SMAP disabled for an
indeterminate amount of time (until they first reload CR4 from
mmu_cr4_features).

> --- 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, const struct cpu_user_regs *regs)

I see you followed the constification request here.

>  static enum pf_type spurious_page_fault(
> -    unsigned long addr, unsigned int error_code)
> +    unsigned long addr, struct cpu_user_regs *regs)

But why not here?

Jan

Re: [PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, May 05, 2014 7:20 PM
> To: Wu, Feng
> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
> Subject: Re: [PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention
> (SMAP) for Xen
> 
> >>> On 05.05.14 at 10:23, <feng.wu@intel.com> wrote:
> > @@ -1394,6 +1401,13 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
> >                          bootstrap_map, cmdline) != 0)
> >          panic("Could not set up DOM0 guest OS");
> >
> > +    /*
> > +     * Enable SMAP after constructing domain0, since there are lots of
> accesses to
> > +     * user pages in construct_dom0(), which is safe at the current stage.
> > +     */
> > +    if ( cpu_has_smap )
> > +        set_in_cr4(X86_CR4_SMAP);
> > +
> 
> I should have noticed this on the previous round already - the way
> it's being done right now will leave APs with SMAP disabled for an
> indeterminate amount of time (until they first reload CR4 from
> mmu_cr4_features).

Yes, that's a question. In that case, we should move this part to the place where
it was. So we should come back to the question about how to handle stac()/clac()
in construct_dom0(), right?


> 
> > --- 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, const struct cpu_user_regs *regs)
> 
> I see you followed the constification request here.
> 
> >  static enum pf_type spurious_page_fault(
> > -    unsigned long addr, unsigned int error_code)
> > +    unsigned long addr, struct cpu_user_regs *regs)
> 
> But why not here?
> 
> Jan

Thanks,
Feng

Re: [PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

[Jan Beulich]

>>> On 06.05.14 at 07:19, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, May 05, 2014 7:20 PM
>> To: Wu, Feng
>> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
>> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org 
>> Subject: Re: [PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention
>> (SMAP) for Xen
>> 
>> >>> On 05.05.14 at 10:23, <feng.wu@intel.com> wrote:
>> > @@ -1394,6 +1401,13 @@ void __init noreturn __start_xen(unsigned long
>> mbi_p)
>> >                          bootstrap_map, cmdline) != 0)
>> >          panic("Could not set up DOM0 guest OS");
>> >
>> > +    /*
>> > +     * Enable SMAP after constructing domain0, since there are lots of
>> accesses to
>> > +     * user pages in construct_dom0(), which is safe at the current stage.
>> > +     */
>> > +    if ( cpu_has_smap )
>> > +        set_in_cr4(X86_CR4_SMAP);
>> > +
>> 
>> I should have noticed this on the previous round already - the way
>> it's being done right now will leave APs with SMAP disabled for an
>> indeterminate amount of time (until they first reload CR4 from
>> mmu_cr4_features).
> 
> Yes, that's a question. In that case, we should move this part to the place 
> where
> it was. So we should come back to the question about how to handle 
> stac()/clac()
> in construct_dom0(), right?

An option might be to move it back to where it was, but clear the flag
temporarily on the one CPU calling construct_dom0(). That's a little
hackish, but properly commented acceptable imo.

Jan

Re: [PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, May 06, 2014 4:02 PM
> To: Wu, Feng
> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
> Subject: RE: [PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention
> (SMAP) for Xen
> 
> >>> On 06.05.14 at 07:19, <feng.wu@intel.com> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, May 05, 2014 7:20 PM
> >> To: Wu, Feng
> >> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
> >> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
> >> Subject: Re: [PATCH v4 08/10] x86: Enable Supervisor Mode Access
> Prevention
> >> (SMAP) for Xen
> >>
> >> >>> On 05.05.14 at 10:23, <feng.wu@intel.com> wrote:
> >> > @@ -1394,6 +1401,13 @@ void __init noreturn __start_xen(unsigned
> long
> >> mbi_p)
> >> >                          bootstrap_map, cmdline) != 0)
> >> >          panic("Could not set up DOM0 guest OS");
> >> >
> >> > +    /*
> >> > +     * Enable SMAP after constructing domain0, since there are lots of
> >> accesses to
> >> > +     * user pages in construct_dom0(), which is safe at the current
> stage.
> >> > +     */
> >> > +    if ( cpu_has_smap )
> >> > +        set_in_cr4(X86_CR4_SMAP);
> >> > +
> >>
> >> I should have noticed this on the previous round already - the way
> >> it's being done right now will leave APs with SMAP disabled for an
> >> indeterminate amount of time (until they first reload CR4 from
> >> mmu_cr4_features).
> >
> > Yes, that's a question. In that case, we should move this part to the place
> > where
> > it was. So we should come back to the question about how to handle
> > stac()/clac()
> > in construct_dom0(), right?
> 
> An option might be to move it back to where it was, but clear the flag
> temporarily on the one CPU calling construct_dom0(). That's a little
> hackish, but properly commented acceptable imo.

Do you mean clear SMAP bit in CR4 before construct_dom0() and set it after that like this?

write_cr4(read_cr4() & ~X86_CR4_SMAP);
construct_dom0();
write_cr4(read_cr4() | X86_CR4_SMAP);

> 
> Jan

Thanks,
Feng

Re: [PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

[Jan Beulich]

>>> On 06.05.14 at 10:24, <feng.wu@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >>> On 06.05.14 at 07:19, <feng.wu@intel.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> I should have noticed this on the previous round already - the way
>> >> it's being done right now will leave APs with SMAP disabled for an
>> >> indeterminate amount of time (until they first reload CR4 from
>> >> mmu_cr4_features).
>> >
>> > Yes, that's a question. In that case, we should move this part to the place
>> > where
>> > it was. So we should come back to the question about how to handle
>> > stac()/clac()
>> > in construct_dom0(), right?
>> 
>> An option might be to move it back to where it was, but clear the flag
>> temporarily on the one CPU calling construct_dom0(). That's a little
>> hackish, but properly commented acceptable imo.
> 
> Do you mean clear SMAP bit in CR4 before construct_dom0() and set it after 
> that like this?
> 
> write_cr4(read_cr4() & ~X86_CR4_SMAP);
> construct_dom0();
> write_cr4(read_cr4() | X86_CR4_SMAP);

Yes, properly conditionalized of course (at least the second one).

Jan

Re: [PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, May 06, 2014 5:11 PM
> To: Wu, Feng
> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
> Subject: RE: [PATCH v4 08/10] x86: Enable Supervisor Mode Access Prevention
> (SMAP) for Xen
> 
> >>> On 06.05.14 at 10:24, <feng.wu@intel.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >>> On 06.05.14 at 07:19, <feng.wu@intel.com> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> I should have noticed this on the previous round already - the way
> >> >> it's being done right now will leave APs with SMAP disabled for an
> >> >> indeterminate amount of time (until they first reload CR4 from
> >> >> mmu_cr4_features).
> >> >
> >> > Yes, that's a question. In that case, we should move this part to the place
> >> > where
> >> > it was. So we should come back to the question about how to handle
> >> > stac()/clac()
> >> > in construct_dom0(), right?
> >>
> >> An option might be to move it back to where it was, but clear the flag
> >> temporarily on the one CPU calling construct_dom0(). That's a little
> >> hackish, but properly commented acceptable imo.
> >
> > Do you mean clear SMAP bit in CR4 before construct_dom0() and set it after
> > that like this?
> >
> > write_cr4(read_cr4() & ~X86_CR4_SMAP);
> > construct_dom0();
> > write_cr4(read_cr4() | X86_CR4_SMAP);
> 
> Yes, properly conditionalized of course (at least the second one).
> 

Yes, need to check cpu_has_smap first. Thanks!

> Jan

Thanks,
Feng