Re: [Xen-devel] [PATCH v5] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself

[Jan Beulich]

>>> On 02.09.16 at 10:20,  wrote:
> +/* 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 ( !parse_bool(s) )
> +{
> +opt_smep = 0;
> +}
> +else if ( !strcmp(s, "hvm") )
> +{
> +opt_smep = SMEP_HVM_ONLY;
> +}
> +
> +if ( opt_smep == 1 )
> +__set_bit(X86_FEATURE_XEN_SMEP, boot_cpu_data.x86_capability);
> +}
> +custom_param("smep", parse_smep_param);

The pointless braces are still there, and it still doesn't look like e.g.
"smep=0 smep=1" would work. Did you take the time to look at
other callers of parse_bool()?

And then - I'm sorry for not having noticed before - setting the
feature flag here means it won't get set if no "smep=" was given.
I.e. you rather want to move that ...

> @@ -1403,12 +1433,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>  if ( !opt_smep )
>  setup_clear_cpu_cap(X86_FEATURE_SMEP);

... around here.

Jan


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

[Xen-devel] [PATCH v5] xen/sm{e, a}p: allow disabling sm{e, a}p for Xen itself

[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.
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 

---
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| 54 -
 xen/include/asm-x86/asm_defns.h | 10 +++
 xen/include/asm-x86/cpufeature.h|  4 +--
 4 files changed, 51 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
 > `= `
@@ -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
 > `=  | cap | `
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 217c775..5256e99 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,44 @@ 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 ( !parse_bool(s) )
+{
+opt_smep = 0;
+}
+else if ( !strcmp(s, "hvm") )
+{
+opt_smep = SMEP_HVM_ONLY;
+}
+
+if ( opt_smep == 1 )
+__set_bit(X86_FEATURE_XEN_SMEP, boot_cpu_data.x86_capability);
+}
+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 ( !parse_bool(s) )
+{
+opt_smap = 0;
+}
+else if ( !strcmp(s, "hvm") )
+{
+opt_smap = SMAP_HVM_ONLY;
+}
+
+if ( opt_smap == 1 )
+__set_bit(X86_FEATURE_XEN_SMAP, boot_cpu_data.x86_capability);
+}
+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 +1433,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 ( 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 )
+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 +1580,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
  * This saves a large number of corner cases interactions with
  * copy_from_user().
  */