Re: [Xen-devel] [PATCH V2] xen: support enabling SMEP/SMAP for HVM only

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.08.16 at 11:17, <he.chen@xxxxxxxxxxxxxxx> wrote:
> Enhance "skajima@xxxxxxxxx>mep" and "smap" command line options to support 

I guess that was meant to be "smep".

> enabling SMEP
> or SMAP for HVM only with allowing "hvm" as a value.

A primary complaint of mine on v1 was not addressed: The description
continues to not explain why what is being done is sufficient.

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1427,19 +1427,21 @@ enabling more sockets and cores to go into deeper 
> sleep states.
>  
>  Set the serial transmit buffer size.
>  
> -### smep
> -> `= <boolean>`
> +### smap
> +> `= <boolean> | hvm`
>  
>  > Default: `true`
>  
> -Flag to enable Supervisor Mode Execution Protection
> +Flag to enable Supervisor Mode Access Prevention
> +Using `smap=hvm` to enable SMAP for HVM guests only.

Use ...

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -4,6 +4,7 @@
>  #include <asm/hvm/hvm.h>
>  #include <asm/hvm/vmx/vmcs.h>
>  #include <asm/processor.h>
> +#include <asm/setup.h>
>  
>  const uint32_t known_features[] = INIT_KNOWN_FEATURES;
>  const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
> @@ -118,6 +119,10 @@ static void __init calculate_pv_featureset(void)
>      __set_bit(X86_FEATURE_HTT, pv_featureset);
>      __set_bit(X86_FEATURE_X2APIC, pv_featureset);
>      __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset);
> +    if ( smep_hvm_only )
> +        __clear_bit(X86_FEATURE_SMEP, pv_featureset);
> +    if ( smap_hvm_only )
> +        __clear_bit(X86_FEATURE_SMAP, pv_featureset);

These flags already can't be set in pv_featureset, so the change
is pointless.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -61,13 +61,19 @@ 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);
> +/* Supervisor Mode Execution Protection (default on). */
> +/* "smep=on": Enable SMEP for Xen and guests.         */
> +/* "smep=hvm": Enable SMEP for HVM only.              */
> +/* "smep=off": Disable SMEP for Xen and guests.       */
> +static void parse_smep_param(char *s);
> +custom_param("smep", parse_smep_param);
> +
> +/* Supervisor Mode Access Prevention (default on). */
> +/* "smep=on": Enable SMAP for Xen and guests.      */
> +/* "smep=hvm": Enable SMAP for HVM only.           */
> +/* "smep=off": Disable SMAP for Xen and guests.    */
> +static void parse_smap_param(char *s);
> +custom_param("smap", parse_smap_param);

The commentary goes too far imo; what's wrong with retaining the
earlier comments? Also please avoid forward declarations of static
functions when you easily can.

> @@ -111,6 +117,34 @@ struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 
> 0, 0, -1 };
>  
>  unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
>  
> +static bool_t __initdata opt_smep = 1;
> +bool_t __initdata smep_hvm_only = 0;

Why two variables when one (of non-boolean type) will do?

> +static void __init parse_smep_param(char *s)
> +{
> +    if ( !parse_bool(s) )
> +    {
> +        opt_smep = 0;
> +    }
> +    else if ( !strcmp(s, "hvm") )
> +    {
> +        smep_hvm_only = 1;
> +    }
> +}

A command line like "smep=hvm smep=on" would no longer work afaict.

Also - stray braces.

> @@ -1404,12 +1438,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      if ( !opt_smep )
>          setup_clear_cpu_cap(X86_FEATURE_SMEP);
>      if ( cpu_has_smep )
> +    {
>          set_in_cr4(X86_CR4_SMEP);
> +        if ( smep_hvm_only )
> +            write_cr4(read_cr4() & ~X86_CR4_SMEP);
> +    }

So that'll clear CR4.SMEP right here, but won't help with CR4 loads
from mmu_cr4_features (as e.g. happens indirectly during VM exits,
since the HOST_CR4 field gets set from this variable).

Did you in fact test your change, including validation of the features
correctly remaining off over the lifetime of the system?

Further, considering that you don't clear the two flags from Xen's
internal feature bitmap, and taken together with the internal feature
bitmap driving alternative instruction patching, I'd assume pointless
(and performance wise perhaps harmful) patching to now take place.

Speaking of performance: The extremely bad performance with the
32-bit PV fix patches in was pretty unexpected. I'd now like to be
double certain that nothing similarly unexpected happens due to the
back on forth between SMEP/SMAP enabled and disabled during VM
entry and exit. One would think there should be no such effect, but
I'd prefer this to be confirmed.

Jan

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