Re: [PATCH 3/3] x86, cpu: Enable/disable SMEP

[Ingo Molnar]

* Fenghua Yu <fenghua.yu@xxxxxxxxx> wrote:

> From: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> 
> Enable/disable newly documented SMEP (Supervisor Mode Execution Protection) CPU
> feature in kernel. CR4.SMEP (bit 20) is 0 at power-on. If the feature is
> supported by CPU (X86_FEATURE_SMEP), enable SMEP by setting CR4.SMEP. New kernel
> option nosmep disables the feature even if the feature is supported by CPU.

Please add a clearer explanation to the changelog and the code as well, 
something like:

   SMEP prevents the CPU in kernel-mode to jump to an executable page that does
   not have the kernel/system flag set in the pte. This prevents the kernel
   from executing user-space code accidentally or maliciously, so it for 
   example prevents kernel exploits from jumping to specially prepared 
   user-mode shell code.

> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> ---
>  Documentation/kernel-parameters.txt |    4 ++++
>  arch/x86/kernel/cpu/common.c        |   21 +++++++++++++++++++++
>  2 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index cc85a92..56fb8c1 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1664,6 +1664,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			noexec=on: enable non-executable mappings (default)
>  			noexec=off: disable non-executable mappings
>  
> +	nosmep		[X86]
> +			Disable SMEP (Supervisor Mode Execution Protection)
> +			even if it is supported by processor.

Typo: s/by processor/by the processor

> +
>  	noexec32	[X86-64]
>  			This affects only 32-bit executables.
>  			noexec32=on: enable non-executable mappings (default)
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index e2ced00..f06b2d5 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -254,6 +254,25 @@ static inline void squash_the_stupid_serial_number(struct cpuinfo_x86 *c)
>  }
>  #endif
>  
> +static int disable_smep;
> +static __init int setup_disable_smep(char *arg)

Nit: please put a newline between variables and the next function.

Also, since setup_smep() is __init this could be __initdata.

> +{
> +	disable_smep = 1;
> +	return 1;
> +}
> +__setup("nosmep", setup_disable_smep);

Naming nit: s/setup_disable_smep/setup_nosmep

> +static __init void setup_smep(struct cpuinfo_x86 *c)
> +{
> +	if (cpu_has(c, X86_FEATURE_SMEP)) {
> +		if (unlikely(disable_smep)) {
> +			setup_clear_cpu_cap(X86_FEATURE_SMEP);
> +			clear_in_cr4(X86_CR4_SMEP);
> +		} else
> +			set_in_cr4(X86_CR4_SMEP);

Nit: Please use symmetric curly braces.

> +	}
> +}
> +
>  /*
>   * Some CPU features depend on higher CPUID levels, which may not always
>   * be available due to CPUID level capping or broken virtualization
> @@ -867,6 +886,8 @@ static void __cpuinit identify_cpu(struct cpuinfo_x86 *c)
>  	/* Init Machine Check Exception if available. */
>  	mcheck_cpu_init(c);
>  
> +	setup_smep(c);
> +
>  	select_idle_routine(c);

The other option would be to enable SMEP in arch/x86/kernel/head_32/64.S where 
we twiddle the cr4 anyway and enable PAE and PGE, and where we do a cpuid to 
check whether the CPU supports NX.

This means we enable SMEP unconditionally on all CPUs that support it, the 
nosmep boot option would turn it off shortly afterwards.

Also, is there some KVM impact of this CPU feature? If the hypervisor can pass 
this to the guest kernel as well then we want to add support there as well, and 
probably want to turn it on by default. (unless it breaks something in a bad 
way)

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/