Re: [PATCH] KVM: VMX: fixes for vmentry_l1d_flush module parameter

[Konrad Rzeszutek Wilk]

On Wed, Aug 22, 2018 at 04:53:00PM +0200, Paolo Bonzini wrote:
> Two bug fixes:
> 
> 1) missing entries in the l1d_param array; this can cause a host crash
> if an access attempts to reach the missing entry. Future-proof the get
> function against any overflows as well.  However, the two entries
> VMENTER_L1D_FLUSH_EPT_DISABLED and VMENTER_L1D_FLUSH_NOT_REQUIRED must
> not be accepted by the parse function, so disable them there.
> 
> 2) invalid values must be rejected even if the CPU does not have the
> bug, so test for them before checking boot_cpu_has(X86_BUG_L1TF)
> 
> ... and a small refactoring, since the .cmd field is redundant with
> the index in the array.
> 
> Reported-by: Bandan Das <bsd@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: a7b9020b06ec6d7c3f3b0d4ef1a9eba12654f4f7
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

with one little nitpick 
> ---
>  arch/x86/kvm/vmx.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c76ca8c4befa..8dae47e7267a 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -198,12 +198,14 @@
>  
>  static const struct {
>  	const char *option;
> -	enum vmx_l1d_flush_state cmd;
> +	bool for_parse;
>  } vmentry_l1d_param[] = {
> -	{"auto",	VMENTER_L1D_FLUSH_AUTO},
> -	{"never",	VMENTER_L1D_FLUSH_NEVER},
> -	{"cond",	VMENTER_L1D_FLUSH_COND},
> -	{"always",	VMENTER_L1D_FLUSH_ALWAYS},
> +	[VMENTER_L1D_FLUSH_AUTO]	 = {"auto", true},
> +	[VMENTER_L1D_FLUSH_NEVER]	 = {"never", true},
> +	[VMENTER_L1D_FLUSH_COND]	 = {"cond", true},
> +	[VMENTER_L1D_FLUSH_ALWAYS]	 = {"always", true},
> +	[VMENTER_L1D_FLUSH_EPT_DISABLED] = {"EPT disabled", false},
> +	[VMENTER_L1D_FLUSH_NOT_REQUIRED] = {"not required", false},

Ingo likes these to have tabs. Any chance you could do:

	[VMENTER_L1D_FLUSH_NOT_REQUIRED] = {"not required",	false},

or so?

>  };
>  
>  #define L1D_CACHE_ORDER 4
> @@ -287,8 +289,9 @@ static int vmentry_l1d_flush_parse(const char *s)
>  
>  	if (s) {
>  		for (i = 0; i < ARRAY_SIZE(vmentry_l1d_param); i++) {
> -			if (sysfs_streq(s, vmentry_l1d_param[i].option))
> -				return vmentry_l1d_param[i].cmd;
> +			if (vmentry_l1d_param[i].for_parse &&
> +			    sysfs_streq(s, vmentry_l1d_param[i].option))
> +				return i;
>  		}
>  	}
>  	return -EINVAL;
> @@ -298,13 +301,13 @@ static int vmentry_l1d_flush_set(const char *s, const struct kernel_param *kp)
>  {
>  	int l1tf, ret;
>  
> -	if (!boot_cpu_has(X86_BUG_L1TF))
> -		return 0;
> -
>  	l1tf = vmentry_l1d_flush_parse(s);
>  	if (l1tf < 0)
>  		return l1tf;
>  
> +	if (!boot_cpu_has(X86_BUG_L1TF))
> +		return 0;
> +
>  	/*
>  	 * Has vmx_init() run already? If not then this is the pre init
>  	 * parameter parsing. In that case just store the value and let
> @@ -324,6 +327,9 @@ static int vmentry_l1d_flush_set(const char *s, const struct kernel_param *kp)
>  
>  static int vmentry_l1d_flush_get(char *s, const struct kernel_param *kp)
>  {
> +	if (WARN_ON_ONCE(l1tf_vmx_mitigation >= ARRAY_SIZE(vmentry_l1d_param)))
> +		return sprintf(s, "???\n");
> +
>  	return sprintf(s, "%s\n", vmentry_l1d_param[l1tf_vmx_mitigation].option);
>  }
>  
> -- 
> 1.8.3.1
>