[PATCH v3] xen: support enabling SMEP/SMAP for HVM only

[PATCH v3] xen: support enabling SMEP/SMAP for HVM only

[He Chen]

SMEP/SMAP is a security feature to prevent kernel executing/accessing
user address involuntarily, any such behavior will lead to a page fault.

SMEP/SMAP is open (in CR4) for both Xen and HVM guest in earlier code.
A 32-bit PV guest will suffer unknown SMEP/SMAP page fault when guest
kernel attempt to access user address although SMEP/SMAP is close for
PV guests already.

This patch is going to support enabling SMEP/SMAP for HVM but disabling
them for Xen hypervisor. Users can choose whether opening them for Xen,
especially when they are going to run 32-bit PV guests.

Signed-off-by: He Chen <he.chen@linux.intel.com>

---
Changes in v3:
* Fix boot options.
* Fix CR4 & mmu_cr4_features operations.
* Disable SMEP/SMAP for Dom0.
* Commit message refinement.

Changes in v2:
* Allow "hvm" as a value to "smep" and "smap" command line options.
* Clear SMEP/SMAP CPUID bits for pv guests if they are set to hvm only.
* Refine docs.
* Rewrite commit message.
---
 docs/misc/xen-command-line.markdown |  2 ++
 xen/arch/x86/setup.c                | 61 ++++++++++++++++++++++++++++++++-----
 xen/arch/x86/traps.c                |  7 +++++
 xen/include/asm-x86/setup.h         |  6 ++++
 4 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 3a250cb..b15f3e7 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1433,6 +1433,7 @@ Set the serial transmit buffer size.
 > Default: `true`
 
 Flag to enable Supervisor Mode Execution Protection
+Use `smep=hvm` to enable SMEP for HVM guests only.
 
 ### smap
 > `= <boolean>`
@@ -1440,6 +1441,7 @@ Flag to enable Supervisor Mode Execution Protection
 > Default: `true`
 
 Flag to enable Supervisor Mode Access Prevention
+Use `smap=hvm` to enable SMAP for HVM guests only.
 
 ### snb\_igd\_quirk
 > `= <boolean> | cap | <integer>`
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 217c775..a428558 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -62,12 +62,12 @@ static unsigned int __initdata max_cpus;
 integer_param("maxcpus", max_cpus);
 
 /* smep: Enable/disable Supervisor Mode Execution Protection (default on). */
-static bool_t __initdata opt_smep = 1;
-boolean_param("smep", opt_smep);
+static void parse_smep_param(char *s);
+custom_param("smep", parse_smep_param);
 
 /* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
-static bool_t __initdata opt_smap = 1;
-boolean_param("smap", opt_smap);
+static void parse_smap_param(char *s);
+custom_param("smap", parse_smap_param);
 
 unsigned long __read_mostly cr4_pv32_mask;
 
@@ -111,6 +111,40 @@ struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
 
 unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
 
+int opt_smep = 1;
+static void __init parse_smep_param(char *s)
+{
+    if ( !strcmp(s, "hvm") )
+    {
+        opt_smep = SMEP_HVM_ONLY;
+    }
+    else if ( !parse_bool(s) )
+    {
+        opt_smep = 0;
+    }
+    else if ( parse_bool(s) && opt_smep != SMEP_HVM_ONLY )
+    {
+        opt_smep = 1;
+    }
+}
+
+int opt_smap = 1;
+static void __init parse_smap_param(char *s)
+{
+    if ( !strcmp(s, "hvm") )
+    {
+        opt_smap = SMAP_HVM_ONLY;
+    }
+    else if ( !parse_bool(s) )
+    {
+        opt_smap = 0;
+    }
+    else if ( parse_bool(s) && opt_smap != SMAP_HVM_ONLY )
+    {
+        opt_smap = 1;
+    }
+}
+
 bool_t __read_mostly acpi_disabled;
 bool_t __initdata acpi_force;
 static char __initdata acpi_param[10] = "";
@@ -1403,12 +1437,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     if ( !opt_smep )
         setup_clear_cpu_cap(X86_FEATURE_SMEP);
-    if ( cpu_has_smep )
+    if ( cpu_has_smep && opt_smep != SMEP_HVM_ONLY )
         set_in_cr4(X86_CR4_SMEP);
 
     if ( !opt_smap )
         setup_clear_cpu_cap(X86_FEATURE_SMAP);
-    if ( cpu_has_smap )
+    if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY )
         set_in_cr4(X86_CR4_SMAP);
 
     cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS;
@@ -1430,8 +1464,19 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     arch_init_memory();
 
+    /*
+     * Temporarily clear SMAP in internal feature bitmap to avoid
+     * patching unnecessary SMAP instructions when SMAP is disabled in
+     * Xen hypervisor.
+     */
+    if ( opt_smap == SMAP_HVM_ONLY )
+        __clear_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
+
     alternative_instructions();
 
+    if ( opt_smap == SMAP_HVM_ONLY )
+        __set_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
+
     local_irq_enable();
 
     pt_pci_init();
@@ -1550,7 +1595,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
      * This saves a large number of corner cases interactions with
      * copy_from_user().
      */
-    if ( cpu_has_smap )
+    if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY )
     {
         cr4_pv32_mask &= ~X86_CR4_SMAP;
         write_cr4(read_cr4() & ~X86_CR4_SMAP);
@@ -1570,7 +1615,7 @@ 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 )
+    if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY )
     {
         write_cr4(read_cr4() | X86_CR4_SMAP);
         cr4_pv32_mask |= X86_CR4_SMAP;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e822719..58989c4 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -77,6 +77,7 @@
 #include <public/arch-x86/cpuid.h>
 #include <asm/cpuid.h>
 #include <xsm/xsm.h>
+#include <asm/setup.h>
 
 /*
  * opt_nmi: one of 'ignore', 'dom0', or 'fatal'.
@@ -1098,6 +1099,12 @@ void pv_cpuid(struct cpu_user_regs *regs)
             b |= (host_featureset[FEATURESET_7b0] &
                   special_features[FEATURESET_7b0]);
 
+            if ( opt_smep == SMEP_HVM_ONLY )
+                b &= ~cpufeat_mask(X86_FEATURE_SMEP);
+
+            if ( opt_smap == SMAP_HVM_ONLY )
+                b &= ~cpufeat_mask(X86_FEATURE_SMAP);
+
             c &= pv_featureset[FEATURESET_7c0];
 
             if ( !is_pvh_domain(currd) )
diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
index c65b79c..7102b5c 100644
--- a/xen/include/asm-x86/setup.h
+++ b/xen/include/asm-x86/setup.h
@@ -51,6 +51,12 @@ void microcode_grab_module(
 
 extern uint8_t kbd_shift_flags;
 
+#define SMEP_HVM_ONLY -1
+extern int opt_smep;
+
+#define SMAP_HVM_ONLY -1
+extern int opt_smap;
+
 #ifdef NDEBUG
 # define highmem_start 0
 #else
-- 
1.9.1


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

Re: [PATCH v3] xen: support enabling SMEP/SMAP for HVM only

[Jan Beulich]

>>> On 19.08.16 at 12:20, <he.chen@linux.intel.com> wrote:
> Changes in v3:
> * Fix boot options.
> * Fix CR4 & mmu_cr4_features operations.
> * Disable SMEP/SMAP for Dom0.
> * Commit message refinement.

Several of my comments on v3 did not get taken care of (neither in
code nor verbally). I'm not going to repeat them here.

> @@ -1403,12 +1437,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      if ( !opt_smep )
>          setup_clear_cpu_cap(X86_FEATURE_SMEP);
> -    if ( cpu_has_smep )
> +    if ( cpu_has_smep && opt_smep != SMEP_HVM_ONLY )
>          set_in_cr4(X86_CR4_SMEP);
>  
>      if ( !opt_smap )
>          setup_clear_cpu_cap(X86_FEATURE_SMAP);
> -    if ( cpu_has_smap )
> +    if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY )
>          set_in_cr4(X86_CR4_SMAP);

So this avoids setting the flags in CR4, but also in mmu_cr4_features.

> @@ -1430,8 +1464,19 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      arch_init_memory();
>  
> +    /*
> +     * Temporarily clear SMAP in internal feature bitmap to avoid
> +     * patching unnecessary SMAP instructions when SMAP is disabled in
> +     * Xen hypervisor.
> +     */
> +    if ( opt_smap == SMAP_HVM_ONLY )
> +        __clear_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
> +
>      alternative_instructions();
>  
> +    if ( opt_smap == SMAP_HVM_ONLY )
> +        __set_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);

I think the better approach would be to introduce a synthetic
feature, which gets set only when SMAP gets used by Xen for
itself. Even if not needed for alternative patching, I think for
symmetry reasons the same should then also be done for SMEP.

> @@ -1098,6 +1099,12 @@ void pv_cpuid(struct cpu_user_regs *regs)
>              b |= (host_featureset[FEATURESET_7b0] &
>                    special_features[FEATURESET_7b0]);
>  
> +            if ( opt_smep == SMEP_HVM_ONLY )
> +                b &= ~cpufeat_mask(X86_FEATURE_SMEP);
> +
> +            if ( opt_smap == SMAP_HVM_ONLY )
> +                b &= ~cpufeat_mask(X86_FEATURE_SMAP);

While you changed the place where you do the adjustment, my
previous comment holds: "These flags already can't be set in
pv_featureset, so the change is pointless."

> --- a/xen/include/asm-x86/setup.h
> +++ b/xen/include/asm-x86/setup.h
> @@ -51,6 +51,12 @@ void microcode_grab_module(
>  
>  extern uint8_t kbd_shift_flags;
>  
> +#define SMEP_HVM_ONLY -1
> +extern int opt_smep;
> +
> +#define SMAP_HVM_ONLY -1
> +extern int opt_smap;

Which then means that these still don't need to become non-static.
The #define-s, if you mean to retain them (in setup.c) would of
course need proper parenthesization.

Jan


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

Re: [PATCH v3] xen: support enabling SMEP/SMAP for HVM only

[He Chen]

On Wed, Aug 24, 2016 at 04:01:53AM -0600, Jan Beulich wrote:
> >>> On 19.08.16 at 12:20, <he.chen@linux.intel.com> wrote:
> > Changes in v3:
> > * Fix boot options.
> > * Fix CR4 & mmu_cr4_features operations.
> > * Disable SMEP/SMAP for Dom0.
> > * Commit message refinement.
> 
> Several of my comments on v3 did not get taken care of (neither in
> code nor verbally). I'm not going to repeat them here.
> 
Let me try to address them well in next patch...

> > @@ -1403,12 +1437,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >  
> >      if ( !opt_smep )
> >          setup_clear_cpu_cap(X86_FEATURE_SMEP);
> > -    if ( cpu_has_smep )
> > +    if ( cpu_has_smep && opt_smep != SMEP_HVM_ONLY )
> >          set_in_cr4(X86_CR4_SMEP);
> >  
> >      if ( !opt_smap )
> >          setup_clear_cpu_cap(X86_FEATURE_SMAP);
> > -    if ( cpu_has_smap )
> > +    if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY )
> >          set_in_cr4(X86_CR4_SMAP);
> 
> So this avoids setting the flags in CR4, but also in mmu_cr4_features.
> 
I am sorry that I am not so clear about this. As what I see in the code,
`mmu_cr4_features` get changed in `set_in_cr4` and `clear_in_cr4` only.
May I ask why the sm{e,a}p is also in `mmu_cr4_features` and where else
it will be set?

> > @@ -1430,8 +1464,19 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >  
> >      arch_init_memory();
> >  
> > +    /*
> > +     * Temporarily clear SMAP in internal feature bitmap to avoid
> > +     * patching unnecessary SMAP instructions when SMAP is disabled in
> > +     * Xen hypervisor.
> > +     */
> > +    if ( opt_smap == SMAP_HVM_ONLY )
> > +        __clear_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
> > +
> >      alternative_instructions();
> >  
> > +    if ( opt_smap == SMAP_HVM_ONLY )
> > +        __set_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
> 
> I think the better approach would be to introduce a synthetic
> feature, which gets set only when SMAP gets used by Xen for
> itself. Even if not needed for alternative patching, I think for
> symmetry reasons the same should then also be done for SMEP.
> 
Here, do you suggest to add a artificial SMAP feature (not from hardware
but according to the `smap` option) bit in `x86_capability` and to patch
SMAP instruction according to this new bit rather than actual hardware
SMAP bit?

Regarding SMEP, even if there are not instructions need to be patched,
but for symmetry reasons we should also add **another** new SMEP bit in
`x86_capability`, right?

> > @@ -1098,6 +1099,12 @@ void pv_cpuid(struct cpu_user_regs *regs)
> >              b |= (host_featureset[FEATURESET_7b0] &
> >                    special_features[FEATURESET_7b0]);
> >  
> > +            if ( opt_smep == SMEP_HVM_ONLY )
> > +                b &= ~cpufeat_mask(X86_FEATURE_SMEP);
> > +
> > +            if ( opt_smap == SMAP_HVM_ONLY )
> > +                b &= ~cpufeat_mask(X86_FEATURE_SMAP);
> 
> While you changed the place where you do the adjustment, my
> previous comment holds: "These flags already can't be set in
> pv_featureset, so the change is pointless."
> 
My carelessness, sorry for this.

> > --- a/xen/include/asm-x86/setup.h
> > +++ b/xen/include/asm-x86/setup.h
> > @@ -51,6 +51,12 @@ void microcode_grab_module(
> >  
> >  extern uint8_t kbd_shift_flags;
> >  
> > +#define SMEP_HVM_ONLY -1
> > +extern int opt_smep;
> > +
> > +#define SMAP_HVM_ONLY -1
> > +extern int opt_smap;
> 
> Which then means that these still don't need to become non-static.
> The #define-s, if you mean to retain them (in setup.c) would of
> course need proper parenthesization.
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH v3] xen: support enabling SMEP/SMAP for HVM only

[Jan Beulich]

>>> On 29.08.16 at 04:47, <he.chen@linux.intel.com> wrote:
> On Wed, Aug 24, 2016 at 04:01:53AM -0600, Jan Beulich wrote:
>> >>> On 19.08.16 at 12:20, <he.chen@linux.intel.com> wrote:
>> > @@ -1403,12 +1437,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>> >  
>> >      if ( !opt_smep )
>> >          setup_clear_cpu_cap(X86_FEATURE_SMEP);
>> > -    if ( cpu_has_smep )
>> > +    if ( cpu_has_smep && opt_smep != SMEP_HVM_ONLY )
>> >          set_in_cr4(X86_CR4_SMEP);
>> >  
>> >      if ( !opt_smap )
>> >          setup_clear_cpu_cap(X86_FEATURE_SMAP);
>> > -    if ( cpu_has_smap )
>> > +    if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY )
>> >          set_in_cr4(X86_CR4_SMAP);
>> 
>> So this avoids setting the flags in CR4, but also in mmu_cr4_features.
>> 
> I am sorry that I am not so clear about this. As what I see in the code,
> `mmu_cr4_features` get changed in `set_in_cr4` and `clear_in_cr4` only.
> May I ask why the sm{e,a}p is also in `mmu_cr4_features` and where else
> it will be set?

I realize that I should have deleted that remark after having checked
VMX code: Not setting the two flags in mmu_cr4_features is actually
what we want. The guest can still set the flags in its view of the
register, independent of what's in mmu_cr4_features.

>> > @@ -1430,8 +1464,19 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>> >  
>> >      arch_init_memory();
>> >  
>> > +    /*
>> > +     * Temporarily clear SMAP in internal feature bitmap to avoid
>> > +     * patching unnecessary SMAP instructions when SMAP is disabled in
>> > +     * Xen hypervisor.
>> > +     */
>> > +    if ( opt_smap == SMAP_HVM_ONLY )
>> > +        __clear_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
>> > +
>> >      alternative_instructions();
>> >  
>> > +    if ( opt_smap == SMAP_HVM_ONLY )
>> > +        __set_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
>> 
>> I think the better approach would be to introduce a synthetic
>> feature, which gets set only when SMAP gets used by Xen for
>> itself. Even if not needed for alternative patching, I think for
>> symmetry reasons the same should then also be done for SMEP.
>> 
> Here, do you suggest to add a artificial SMAP feature (not from hardware
> but according to the `smap` option) bit in `x86_capability` and to patch
> SMAP instruction according to this new bit rather than actual hardware
> SMAP bit?

Yes.

> Regarding SMEP, even if there are not instructions need to be patched,
> but for symmetry reasons we should also add **another** new SMEP bit in
> `x86_capability`, right?

Yes.

Jan


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