All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself
@ 2016-09-05  5:17 He Chen
  2016-09-05 10:06 ` Jan Beulich
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: He Chen @ 2016-09-05  5:17 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Feng Wu, Jan Beulich

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.
SMEP/SMAP bit set in Xen CR4 would enforce security checking for 32-bit
PV guest which will suffer unknown SMEP/SMAP page fault when guest
kernel attempt to access user address although SMEP/SMAP is close for
PV guests.

This patch introduces a new boot option value "hvm" for "sm{e,a}p", it
is going to diable SMEP/SMAP for Xen hypervisor while enable them for
HVM. In this way, 32-bit PV guest will not suffer SMEP/SMAP security
issue. Users can choose whether open SMEP/SMAP for Xen itself,
especially when they are going to run 32-bit PV guests.

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

---
Changes in v6:
* fix sm{e,a}p parameters parser flow.

Changes in v5:
* refine sm{e,a}p parameters parser flow.
* replace cpu_has_sm{e,a}p with boot_cpu_has(X86_FEATURE_XEN_SM{E,A}P).
* refine docs.

Changes in v4:
* introduce 2 new synthetic features X86_FEATURE_XEN_SMEP and
  X86_FEATURE_XEN_SMAP for Xen itself.
* adjust SM{E,A}P related instruction patching code.
* commit message refinement.

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                | 76 +++++++++++++++++++++++++++++++------
 xen/include/asm-x86/asm_defns.h     | 10 ++---
 xen/include/asm-x86/cpufeature.h    |  4 +-
 4 files changed, 73 insertions(+), 19 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 3a250cb..0225974 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 allow SMEP use by 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 allow SMAP use by 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..3c41715 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -61,14 +61,6 @@ boolean_param("nosmp", opt_nosmp);
 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);
-
-/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
-static bool_t __initdata opt_smap = 1;
-boolean_param("smap", opt_smap);
-
 unsigned long __read_mostly cr4_pv32_mask;
 
 /* Boot dom0 in pvh mode */
@@ -111,6 +103,62 @@ struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
 
 unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
 
+/* smep: Enable/disable Supervisor Mode Execution Protection (default on). */
+#define SMEP_HVM_ONLY (-1)
+static s8 __initdata opt_smep = 1;
+static void __init parse_smep_param(char *s)
+{
+    if ( !*s )
+    {
+        opt_smep = 1;
+        return;
+    }
+
+    switch ( parse_bool(s) )
+    {
+    case 0:
+        opt_smep = 0;
+        return;
+    case 1:
+        opt_smep = 1;
+        return;
+    }
+
+    if ( !strcmp(s, "hvm") )
+    {
+        opt_smep = SMEP_HVM_ONLY;
+    }
+}
+custom_param("smep", parse_smep_param);
+
+/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
+#define SMAP_HVM_ONLY (-1)
+static s8 __initdata opt_smap = 1;
+static void __init parse_smap_param(char *s)
+{
+    if ( !*s )
+    {
+        opt_smap = 1;
+        return;
+    }
+
+    switch ( parse_bool(s) )
+    {
+    case 0:
+        opt_smap = 0;
+        return;
+    case 1:
+        opt_smap = 1;
+        return;
+    }
+
+    if ( !strcmp(s, "hvm") )
+    {
+        opt_smap = SMAP_HVM_ONLY;
+    }
+}
+custom_param("smap", parse_smap_param);
+
 bool_t __read_mostly acpi_disabled;
 bool_t __initdata acpi_force;
 static char __initdata acpi_param[10] = "";
@@ -1403,12 +1451,16 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     if ( !opt_smep )
         setup_clear_cpu_cap(X86_FEATURE_SMEP);
-    if ( cpu_has_smep )
+    else if ( opt_smep == 1 )
+        __set_bit(X86_FEATURE_XEN_SMEP, boot_cpu_data.x86_capability);
+    if ( boot_cpu_has(X86_FEATURE_XEN_SMEP) )
         set_in_cr4(X86_CR4_SMEP);
 
     if ( !opt_smap )
         setup_clear_cpu_cap(X86_FEATURE_SMAP);
-    if ( cpu_has_smap )
+    else if ( opt_smap == 1 )
+        __set_bit(X86_FEATURE_XEN_SMAP, boot_cpu_data.x86_capability);
+    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
         set_in_cr4(X86_CR4_SMAP);
 
     cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS;
@@ -1550,7 +1602,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 ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
     {
         cr4_pv32_mask &= ~X86_CR4_SMAP;
         write_cr4(read_cr4() & ~X86_CR4_SMAP);
@@ -1570,7 +1622,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 ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
     {
         write_cr4(read_cr4() | X86_CR4_SMAP);
         cr4_pv32_mask |= X86_CR4_SMAP;
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index e36e78f..f1c6fa1 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -205,7 +205,7 @@ void ret_from_intr(void);
         .popsection;                                                   \
         .pushsection .altinstructions, "a";                            \
         altinstruction_entry 661b, 661b, X86_FEATURE_ALWAYS, 3, 0;     \
-        altinstruction_entry 661b, 662b, X86_FEATURE_SMAP, 3, 3;       \
+        altinstruction_entry 661b, 662b, X86_FEATURE_XEN_SMAP, 3, 3;       \
         .popsection
 
 #define ASM_STAC ASM_AC(STAC)
@@ -217,21 +217,21 @@ void ret_from_intr(void);
         668: call cr4_pv32_restore;                                \
         .section .altinstructions, "a";                            \
         altinstruction_entry 667b, 667b, X86_FEATURE_ALWAYS, 5, 0; \
-        altinstruction_entry 667b, 668b, X86_FEATURE_SMEP, 5, 5;   \
-        altinstruction_entry 667b, 668b, X86_FEATURE_SMAP, 5, 5;   \
+        altinstruction_entry 667b, 668b, X86_FEATURE_XEN_SMEP, 5, 5;   \
+        altinstruction_entry 667b, 668b, X86_FEATURE_XEN_SMAP, 5, 5;   \
         .popsection
 
 #else
 static always_inline void clac(void)
 {
     /* Note: a barrier is implicit in alternative() */
-    alternative(ASM_NOP3, __stringify(__ASM_CLAC), X86_FEATURE_SMAP);
+    alternative(ASM_NOP3, __stringify(__ASM_CLAC), X86_FEATURE_XEN_SMAP);
 }
 
 static always_inline void stac(void)
 {
     /* Note: a barrier is implicit in alternative() */
-    alternative(ASM_NOP3, __stringify(__ASM_STAC), X86_FEATURE_SMAP);
+    alternative(ASM_NOP3, __stringify(__ASM_STAC), X86_FEATURE_XEN_SMAP);
 }
 #endif
 
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index bcdf5d6..0f6810a 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -17,6 +17,8 @@ XEN_CPUFEATURE(CPUID_FAULTING,  (FSCAPINTS+0)*32+ 6) /* cpuid faulting */
 XEN_CPUFEATURE(CLFLUSH_MONITOR, (FSCAPINTS+0)*32+ 7) /* clflush reqd with monitor */
 XEN_CPUFEATURE(APERFMPERF,      (FSCAPINTS+0)*32+ 8) /* APERFMPERF */
 XEN_CPUFEATURE(MFENCE_RDTSC,    (FSCAPINTS+0)*32+ 9) /* MFENCE synchronizes RDTSC */
+XEN_CPUFEATURE(XEN_SMEP,        (FSCAPINTS+0)*32+ 10) /* SMEP gets used by Xen itself */
+XEN_CPUFEATURE(XEN_SMAP,        (FSCAPINTS+0)*32+ 11) /* SMAP gets used by Xen itself */
 
 #define NCAPINTS (FSCAPINTS + 1) /* N 32-bit words worth of info */
 
@@ -66,8 +68,6 @@ XEN_CPUFEATURE(MFENCE_RDTSC,    (FSCAPINTS+0)*32+ 9) /* MFENCE synchronizes RDTS
 #define cpu_has_page1gb		boot_cpu_has(X86_FEATURE_PAGE1GB)
 #define cpu_has_fsgsbase	boot_cpu_has(X86_FEATURE_FSGSBASE)
 #define cpu_has_aperfmperf	boot_cpu_has(X86_FEATURE_APERFMPERF)
-#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) \
                                  && boot_cpu_has(X86_FEATURE_FFXSR))
-- 
1.9.1


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself
  2016-09-05  5:17 [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself He Chen
@ 2016-09-05 10:06 ` Jan Beulich
  2016-09-05 13:07 ` Jan Beulich
  2016-09-20  2:29 ` He Chen
  2 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2016-09-05 10:06 UTC (permalink / raw)
  To: He Chen; +Cc: Andrew Cooper, Feng Wu, xen-devel

>>> On 05.09.16 at 07:17, <he.chen@linux.intel.com> wrote:
> 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.
> SMEP/SMAP bit set in Xen CR4 would enforce security checking for 32-bit
> PV guest which will suffer unknown SMEP/SMAP page fault when guest
> kernel attempt to access user address although SMEP/SMAP is close for
> PV guests.
> 
> This patch introduces a new boot option value "hvm" for "sm{e,a}p", it
> is going to diable SMEP/SMAP for Xen hypervisor while enable them for
> HVM. In this way, 32-bit PV guest will not suffer SMEP/SMAP security
> issue. Users can choose whether open SMEP/SMAP for Xen itself,
> especially when they are going to run 32-bit PV guests.
> 
> Signed-off-by: He Chen <he.chen@linux.intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

albeit one style issue still wasn't taken care of (I'll try to remember
to clean this up when committing):

> @@ -111,6 +103,62 @@ struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
>  
>  unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
>  
> +/* smep: Enable/disable Supervisor Mode Execution Protection (default on). */
> +#define SMEP_HVM_ONLY (-1)
> +static s8 __initdata opt_smep = 1;
> +static void __init parse_smep_param(char *s)
> +{
> +    if ( !*s )
> +    {
> +        opt_smep = 1;
> +        return;
> +    }
> +
> +    switch ( parse_bool(s) )
> +    {
> +    case 0:
> +        opt_smep = 0;
> +        return;
> +    case 1:
> +        opt_smep = 1;
> +        return;
> +    }
> +
> +    if ( !strcmp(s, "hvm") )
> +    {
> +        opt_smep = SMEP_HVM_ONLY;
> +    }

You still left unnecessary braces here.

Jan


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself
  2016-09-05  5:17 [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself He Chen
  2016-09-05 10:06 ` Jan Beulich
@ 2016-09-05 13:07 ` Jan Beulich
  2016-09-20  2:29 ` He Chen
  2 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2016-09-05 13:07 UTC (permalink / raw)
  To: He Chen; +Cc: Andrew Cooper, Feng Wu, xen-devel

>>> On 05.09.16 at 07:17, <he.chen@linux.intel.com> wrote:
> @@ -1403,12 +1451,16 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      if ( !opt_smep )
>          setup_clear_cpu_cap(X86_FEATURE_SMEP);
> -    if ( cpu_has_smep )
> +    else if ( opt_smep == 1 )
> +        __set_bit(X86_FEATURE_XEN_SMEP, boot_cpu_data.x86_capability);
> +    if ( boot_cpu_has(X86_FEATURE_XEN_SMEP) )
>          set_in_cr4(X86_CR4_SMEP);
>  
>      if ( !opt_smap )
>          setup_clear_cpu_cap(X86_FEATURE_SMAP);
> -    if ( cpu_has_smap )
> +    else if ( opt_smap == 1 )
> +        __set_bit(X86_FEATURE_XEN_SMAP, boot_cpu_data.x86_capability);
> +    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
>          set_in_cr4(X86_CR4_SMAP);

This is still wrong, as spotted by osstest's smoke test: It in particular
doesn't work on a system which doesn't have SMEP and/or SMAP.
Please fix this while incorporating the other adjustments I did while
committing; I've reverted the patch until then.

Jan


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself
  2016-09-05  5:17 [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself He Chen
  2016-09-05 10:06 ` Jan Beulich
  2016-09-05 13:07 ` Jan Beulich
@ 2016-09-20  2:29 ` He Chen
  2016-09-20  6:53   ` Jan Beulich
  2 siblings, 1 reply; 6+ messages in thread
From: He Chen @ 2016-09-20  2:29 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Andrew Cooper, Paul Lai, Feng Wu, xen-devel

Hi Jan,

Sorry for the late response, I saw the this patch was merged but soon
got reverted, and the revert message says this patch is still buggy.

I would be most grateful if you would point out the buggy part of this
patch and the reason why revert it.

Thanks,
-He

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself
  2016-09-20  2:29 ` He Chen
@ 2016-09-20  6:53   ` Jan Beulich
  2016-09-20  7:26     ` He Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2016-09-20  6:53 UTC (permalink / raw)
  To: He Chen; +Cc: Andrew Cooper, Paul Lai, Feng Wu, xen-devel

>>> On 20.09.16 at 04:29, <he.chen@linux.intel.com> wrote:
> Sorry for the late response, I saw the this patch was merged but soon
> got reverted, and the revert message says this patch is still buggy.
> 
> I would be most grateful if you would point out the buggy part of this
> patch and the reason why revert it.

Well, I've already told you on the 5th - see
https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00414.html
suggesting that you never tested the patch on older hardware.
Additionally you could (and perhaps should) have looked yourself
immediately at the failed smoke test logs (they may have got purged
by now), referenced from the respective osstest mail
https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00411.html

Jan


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself
  2016-09-20  6:53   ` Jan Beulich
@ 2016-09-20  7:26     ` He Chen
  0 siblings, 0 replies; 6+ messages in thread
From: He Chen @ 2016-09-20  7:26 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Andrew Cooper, Paul Lai, Feng Wu, xen-devel

On Tue, Sep 20, 2016 at 12:53:32AM -0600, Jan Beulich wrote:
> >>> On 20.09.16 at 04:29, <he.chen@linux.intel.com> wrote:
> > Sorry for the late response, I saw the this patch was merged but soon
> > got reverted, and the revert message says this patch is still buggy.
> > 
> > I would be most grateful if you would point out the buggy part of this
> > patch and the reason why revert it.
> 
> Well, I've already told you on the 5th - see
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00414.html
> suggesting that you never tested the patch on older hardware.
> Additionally you could (and perhaps should) have looked yourself
> immediately at the failed smoke test logs (they may have got purged
> by now), referenced from the respective osstest mail
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00411.html

Oops, I didn't receive your reply at 5th Sep due to our mail server
temporarily down that day, terribly sorry..
Thank you for replying again and I will cook a patch and test it asap...

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-09-20  7:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-05  5:17 [PATCH v6] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself He Chen
2016-09-05 10:06 ` Jan Beulich
2016-09-05 13:07 ` Jan Beulich
2016-09-20  2:29 ` He Chen
2016-09-20  6:53   ` Jan Beulich
2016-09-20  7:26     ` He Chen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.