kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] KVM: VMX: fixes for vmentry_l1d_flush module parameter
@ 2018-08-22 14:53 Paolo Bonzini
  2018-08-22 15:29 ` Konrad Rzeszutek Wilk
  2018-08-22 15:31 ` Jack Wang
  0 siblings, 2 replies; 4+ messages in thread
From: Paolo Bonzini @ 2018-08-22 14:53 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: bsd, stable

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@redhat.com>
Cc: stable@vger.kernel.org
Fixes: a7b9020b06ec6d7c3f3b0d4ef1a9eba12654f4f7
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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},
 };
 
 #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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: VMX: fixes for vmentry_l1d_flush module parameter
  2018-08-22 14:53 [PATCH] KVM: VMX: fixes for vmentry_l1d_flush module parameter Paolo Bonzini
@ 2018-08-22 15:29 ` Konrad Rzeszutek Wilk
  2018-08-22 18:02   ` Paolo Bonzini
  2018-08-22 15:31 ` Jack Wang
  1 sibling, 1 reply; 4+ messages in thread
From: Konrad Rzeszutek Wilk @ 2018-08-22 15:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, bsd, stable

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@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: a7b9020b06ec6d7c3f3b0d4ef1a9eba12654f4f7
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: VMX: fixes for vmentry_l1d_flush module parameter
  2018-08-22 14:53 [PATCH] KVM: VMX: fixes for vmentry_l1d_flush module parameter Paolo Bonzini
  2018-08-22 15:29 ` Konrad Rzeszutek Wilk
@ 2018-08-22 15:31 ` Jack Wang
  1 sibling, 0 replies; 4+ messages in thread
From: Jack Wang @ 2018-08-22 15:31 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, bsd, stable

Paolo Bonzini <pbonzini@redhat.com> 于2018年8月22日周三 下午4:56写道:
>
> 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@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: a7b9020b06ec6d7c3f3b0d4ef1a9eba12654f4f7
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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},
>  };
>
>  #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
>
Tested-by: Jack Wang <jinpu.wang@profitbricks.com>
Thanks,

Jack

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: VMX: fixes for vmentry_l1d_flush module parameter
  2018-08-22 15:29 ` Konrad Rzeszutek Wilk
@ 2018-08-22 18:02   ` Paolo Bonzini
  0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2018-08-22 18:02 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: linux-kernel, kvm, bsd, stable

On 22/08/2018 17:29, Konrad Rzeszutek Wilk wrote:
>> +	[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?
> 

Hmm, too late...  Sorry about that.

Paolo

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-08-22 18:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-22 14:53 [PATCH] KVM: VMX: fixes for vmentry_l1d_flush module parameter Paolo Bonzini
2018-08-22 15:29 ` Konrad Rzeszutek Wilk
2018-08-22 18:02   ` Paolo Bonzini
2018-08-22 15:31 ` Jack Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).