Re: [Xen-devel] [PATCH] x86/sm{e, a}p: do not enable SMEP/SMAP in PV shim by default on AMD

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jan 16, 2020 at 04:00:03PM +0000, Igor Druzhinin wrote:
> Due to AMD and Hygon being unable to selectively trap CR4 bit modifications
> running 32-bit PV guest inside PV shim comes with significant performance
> hit. Moreover, for SMEP in particular every time CR4.SMEP changes on context
> switch to/from 32-bit PV guest, it gets trapped by L0 Xen which then
> tries to perform global TLB invalidation for PV shim domain. This usually
> results in eventual hang of a PV shim with at least several vCPUs.
> 
> Since the overall security risk is generally lower for shim Xen as it being
> there more of a defense-in-depth mechanism, choose to disable SMEP/SMAP in
> it by default on AMD and Hygon unless a user chose otherwise.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> ---
> I'm a little bit on the fence with this one. We're having the same issue with
> general nested virt but I'm not inclined to trade security for a user in
> general case. Disabling it by default for PV shim only seems rather inocuous
> due to the use case restricion. I'd like to hear more opinions.
> ---
>  docs/misc/xen-command-line.pandoc | 10 ++++++++--
>  xen/arch/x86/setup.c              | 20 ++++++++++++++------
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 981a5e2..05b2dde 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1936,19 +1936,25 @@ is 1MB.
>  ### smap (x86)
>  > `= <boolean> | hvm`
>  
> -> Default: `true`
> +> Default: `true` unless running in pv-shim mode on AMD or Hygon hardware
>  
>  Flag to enable Supervisor Mode Access Prevention
>  Use `smap=hvm` to allow SMAP use by HVM guests only.
>  
> +In PV shim mode on AMD or Hygon hardware due to significant perfomance impact
> +in some cases and generally lower security risk the option defaults to false.
> +
>  ### smep (x86)
>  > `= <boolean> | hvm`
>  
> -> Default: `true`
> +> Default: `true` unless running in pv-shim mode on AMD or Hygon hardware
>  
>  Flag to enable Supervisor Mode Execution Protection
>  Use `smep=hvm` to allow SMEP use by HVM guests only.
>  
> +In PV shim mode on AMD or Hygon hardware due to significant perfomance impact
> +in some cases and generally lower security risk the option defaults to false.
> +
>  ### smt (x86)
>  > `= <boolean>`
>  
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 5bdc229..8432b77 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -105,9 +105,9 @@ 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;
> +/* smep: Enable/disable Supervisor Mode Execution Protection */
> +#define SMEP_HVM_ONLY (-2)
> +static s8 __initdata opt_smep = -1;

Could you change the type to int8_t instead of s8? (here and below,
can be done on commit with the changes requested by Jan).

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel