public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] KVM: VMX: Mark Intel PT virtualization as BROKEN
@ 2024-11-01 18:50 Sean Christopherson
  2024-11-01 18:50 ` [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN Sean Christopherson
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-11-01 18:50 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Adrian Hunter

Hide Intel PT virtualization behind BROKEN, as it has multiple fatal
flaws, several which put the host at risk, and several of which are far
too invasive to backport to stable trees.

I included one of the easier fixes from Adrian to help show just how
broken PT virtualization is, e.g. to illustrate the apparent lack of usage.

Adrian Hunter (1):
  KVM: VMX: Allow toggling bits in MSR_IA32_RTIT_CTL when enable bit is
    cleared

Sean Christopherson (1):
  KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind
    CONFIG_BROKEN

 arch/x86/kvm/vmx/vmx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)


base-commit: e466901b947d529f7b091a3b00b19d2bdee206ee
-- 
2.47.0.163.g1226f6d8fa-goog


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

* [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN
  2024-11-01 18:50 [PATCH 0/2] KVM: VMX: Mark Intel PT virtualization as BROKEN Sean Christopherson
@ 2024-11-01 18:50 ` Sean Christopherson
  2024-11-04  1:56   ` Xiaoyao Li
                     ` (2 more replies)
  2024-11-01 18:50 ` [PATCH 2/2] KVM: VMX: Allow toggling bits in MSR_IA32_RTIT_CTL when enable bit is cleared Sean Christopherson
  2024-12-19  2:41 ` [PATCH 0/2] KVM: VMX: Mark Intel PT virtualization as BROKEN Sean Christopherson
  2 siblings, 3 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-11-01 18:50 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Adrian Hunter

Hide KVM's pt_mode module param behind CONFIG_BROKEN, i.e. disable support
for virtualizing Intel PT via guest/host mode unless BROKEN=y.  There are
myriad bugs in the implementation, some of which are fatal to the guest,
and others which put the stability and health of the host at risk.

For guest fatalities, the most glaring issue is that KVM fails to ensure
tracing is disabled, and *stays* disabled prior to VM-Enter, which is
necessary as hardware disallows loading (the guest's) RTIT_CTL if tracing
is enabled (enforced via a VMX consistency check).  Per the SDM:

  If the logical processor is operating with Intel PT enabled (if
  IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the "load
  IA32_RTIT_CTL" VM-entry control must be 0.

On the host side, KVM doesn't validate the guest CPUID configuration
provided by userspace, and even worse, uses the guest configuration to
decide what MSRs to save/load at VM-Enter and VM-Exit.  E.g. configuring
guest CPUID to enumerate more address ranges than are supported in hardware
will result in KVM trying to passthrough, save, and load non-existent MSRs,
which generates a variety of WARNs, ToPA ERRORs in the host, a potential
deadlock, etc.

Fixes: f99e3daf94ff ("KVM: x86: Add Intel PT virtualization work mode")
Cc: stable@vger.kernel.org
Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6ed801ffe33f..087504fb1589 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -217,9 +217,11 @@ module_param(ple_window_shrink, uint, 0444);
 static unsigned int ple_window_max        = KVM_VMX_DEFAULT_PLE_WINDOW_MAX;
 module_param(ple_window_max, uint, 0444);
 
-/* Default is SYSTEM mode, 1 for host-guest mode */
+/* Default is SYSTEM mode, 1 for host-guest mode (which is BROKEN) */
 int __read_mostly pt_mode = PT_MODE_SYSTEM;
+#ifdef CONFIG_BROKEN
 module_param(pt_mode, int, S_IRUGO);
+#endif
 
 struct x86_pmu_lbr __ro_after_init vmx_lbr_caps;
 
-- 
2.47.0.163.g1226f6d8fa-goog


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

* [PATCH 2/2] KVM: VMX: Allow toggling bits in MSR_IA32_RTIT_CTL when enable bit is cleared
  2024-11-01 18:50 [PATCH 0/2] KVM: VMX: Mark Intel PT virtualization as BROKEN Sean Christopherson
  2024-11-01 18:50 ` [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN Sean Christopherson
@ 2024-11-01 18:50 ` Sean Christopherson
  2024-11-04  1:57   ` Xiaoyao Li
  2024-12-19  2:41 ` [PATCH 0/2] KVM: VMX: Mark Intel PT virtualization as BROKEN Sean Christopherson
  2 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-11-01 18:50 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Adrian Hunter

From: Adrian Hunter <adrian.hunter@intel.com>

Allow toggling other bits in MSR_IA32_RTIT_CTL if the enable bit is being
cleared, the existing logic simply ignores the enable bit.  E.g. KVM will
incorrectly reject a write of '0' to stop tracing.

Fixes: bf8c55d8dc09 ("KVM: x86: Implement Intel PT MSRs read/write emulation")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
[sean: rework changelog, drop stable@]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 087504fb1589..9b9d115c4824 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1636,7 +1636,8 @@ static int vmx_rtit_ctl_check(struct kvm_vcpu *vcpu, u64 data)
 	 * result in a #GP unless the same write also clears TraceEn.
 	 */
 	if ((vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) &&
-		((vmx->pt_desc.guest.ctl ^ data) & ~RTIT_CTL_TRACEEN))
+	    (data & RTIT_CTL_TRACEEN) &&
+	    data != vmx->pt_desc.guest.ctl)
 		return 1;
 
 	/*
-- 
2.47.0.163.g1226f6d8fa-goog


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

* Re: [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN
  2024-11-01 18:50 ` [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN Sean Christopherson
@ 2024-11-04  1:56   ` Xiaoyao Li
  2024-11-04 22:46     ` Sean Christopherson
  2024-11-04  8:23   ` Adrian Hunter
  2024-11-08  9:07   ` Paolo Bonzini
  2 siblings, 1 reply; 11+ messages in thread
From: Xiaoyao Li @ 2024-11-04  1:56 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Adrian Hunter

On 11/2/2024 2:50 AM, Sean Christopherson wrote:
> Hide KVM's pt_mode module param behind CONFIG_BROKEN, i.e. disable support
> for virtualizing Intel PT via guest/host mode unless BROKEN=y.  There are
> myriad bugs in the implementation, some of which are fatal to the guest,
> and others which put the stability and health of the host at risk.
> 
> For guest fatalities, the most glaring issue is that KVM fails to ensure
> tracing is disabled, and *stays* disabled prior to VM-Enter, which is
> necessary as hardware disallows loading (the guest's) RTIT_CTL if tracing
> is enabled (enforced via a VMX consistency check).  Per the SDM:
> 
>    If the logical processor is operating with Intel PT enabled (if
>    IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the "load
>    IA32_RTIT_CTL" VM-entry control must be 0.
> 
> On the host side, KVM doesn't validate the guest CPUID configuration
> provided by userspace, and even worse, uses the guest configuration to
> decide what MSRs to save/load at VM-Enter and VM-Exit.  E.g. configuring
> guest CPUID to enumerate more address ranges than are supported in hardware
> will result in KVM trying to passthrough, save, and load non-existent MSRs,
> which generates a variety of WARNs, ToPA ERRORs in the host, a potential
> deadlock, etc.
> 
> Fixes: f99e3daf94ff ("KVM: x86: Add Intel PT virtualization work mode")
> Cc: stable@vger.kernel.org
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/vmx/vmx.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6ed801ffe33f..087504fb1589 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -217,9 +217,11 @@ module_param(ple_window_shrink, uint, 0444);
>   static unsigned int ple_window_max        = KVM_VMX_DEFAULT_PLE_WINDOW_MAX;
>   module_param(ple_window_max, uint, 0444);
>   
> -/* Default is SYSTEM mode, 1 for host-guest mode */
> +/* Default is SYSTEM mode, 1 for host-guest mode (which is BROKEN) */
>   int __read_mostly pt_mode = PT_MODE_SYSTEM;
> +#ifdef CONFIG_BROKEN
>   module_param(pt_mode, int, S_IRUGO);
> +#endif

I like the patch, but I didn't find any other usercase of CONFIG_BROKEN 
in current Linux.

>   struct x86_pmu_lbr __ro_after_init vmx_lbr_caps;
>   


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

* Re: [PATCH 2/2] KVM: VMX: Allow toggling bits in MSR_IA32_RTIT_CTL when enable bit is cleared
  2024-11-01 18:50 ` [PATCH 2/2] KVM: VMX: Allow toggling bits in MSR_IA32_RTIT_CTL when enable bit is cleared Sean Christopherson
@ 2024-11-04  1:57   ` Xiaoyao Li
  0 siblings, 0 replies; 11+ messages in thread
From: Xiaoyao Li @ 2024-11-04  1:57 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Adrian Hunter

On 11/2/2024 2:50 AM, Sean Christopherson wrote:
> From: Adrian Hunter <adrian.hunter@intel.com>
> 
> Allow toggling other bits in MSR_IA32_RTIT_CTL if the enable bit is being
> cleared, the existing logic simply ignores the enable bit.  E.g. KVM will
> incorrectly reject a write of '0' to stop tracing.

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> Fixes: bf8c55d8dc09 ("KVM: x86: Implement Intel PT MSRs read/write emulation")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> [sean: rework changelog, drop stable@]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/vmx/vmx.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 087504fb1589..9b9d115c4824 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1636,7 +1636,8 @@ static int vmx_rtit_ctl_check(struct kvm_vcpu *vcpu, u64 data)
>   	 * result in a #GP unless the same write also clears TraceEn.
>   	 */
>   	if ((vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) &&
> -		((vmx->pt_desc.guest.ctl ^ data) & ~RTIT_CTL_TRACEEN))
> +	    (data & RTIT_CTL_TRACEEN) &&
> +	    data != vmx->pt_desc.guest.ctl)
>   		return 1;
>   
>   	/*


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

* Re: [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN
  2024-11-01 18:50 ` [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN Sean Christopherson
  2024-11-04  1:56   ` Xiaoyao Li
@ 2024-11-04  8:23   ` Adrian Hunter
  2024-11-04 23:52     ` Sean Christopherson
  2024-11-08  9:07   ` Paolo Bonzini
  2 siblings, 1 reply; 11+ messages in thread
From: Adrian Hunter @ 2024-11-04  8:23 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel

On 1/11/24 20:50, Sean Christopherson wrote:
> Hide KVM's pt_mode module param behind CONFIG_BROKEN, i.e. disable support
> for virtualizing Intel PT via guest/host mode unless BROKEN=y.  There are
> myriad bugs in the implementation, some of which are fatal to the guest,
> and others which put the stability and health of the host at risk.
> 
> For guest fatalities, the most glaring issue is that KVM fails to ensure
> tracing is disabled, and *stays* disabled prior to VM-Enter, which is
> necessary as hardware disallows loading (the guest's) RTIT_CTL if tracing
> is enabled (enforced via a VMX consistency check).  Per the SDM:
> 
>   If the logical processor is operating with Intel PT enabled (if
>   IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the "load
>   IA32_RTIT_CTL" VM-entry control must be 0.
> 
> On the host side, KVM doesn't validate the guest CPUID configuration
> provided by userspace, and even worse, uses the guest configuration to
> decide what MSRs to save/load at VM-Enter and VM-Exit.  E.g. configuring
> guest CPUID to enumerate more address ranges than are supported in hardware
> will result in KVM trying to passthrough, save, and load non-existent MSRs,
> which generates a variety of WARNs, ToPA ERRORs in the host, a potential
> deadlock, etc.
> 
> Fixes: f99e3daf94ff ("KVM: x86: Add Intel PT virtualization work mode")
> Cc: stable@vger.kernel.org
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6ed801ffe33f..087504fb1589 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -217,9 +217,11 @@ module_param(ple_window_shrink, uint, 0444);
>  static unsigned int ple_window_max        = KVM_VMX_DEFAULT_PLE_WINDOW_MAX;
>  module_param(ple_window_max, uint, 0444);
>  
> -/* Default is SYSTEM mode, 1 for host-guest mode */
> +/* Default is SYSTEM mode, 1 for host-guest mode (which is BROKEN) */
>  int __read_mostly pt_mode = PT_MODE_SYSTEM;
> +#ifdef CONFIG_BROKEN
>  module_param(pt_mode, int, S_IRUGO);
> +#endif

Side effects are:
1. If pt_mode is passed via modprobe, there will be a warning in kernel messages:
	kvm_intel: unknown parameter 'pt_mode' ignored
2. The sysfs module parameter file pt_mode will be gone:
	# cat /sys/module/kvm_intel/parameters/pt_mode
	cat: /sys/module/kvm_intel/parameters/pt_mode: No such file or directory

Nevertheless:

Tested-by: Adrian Hunter <adrian.hunter@intel.com>

>  
>  struct x86_pmu_lbr __ro_after_init vmx_lbr_caps;
>  


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

* Re: [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN
  2024-11-04  1:56   ` Xiaoyao Li
@ 2024-11-04 22:46     ` Sean Christopherson
  2024-11-05 13:34       ` Xiaoyao Li
  0 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-11-04 22:46 UTC (permalink / raw)
  To: Xiaoyao Li; +Cc: Paolo Bonzini, kvm, linux-kernel, Adrian Hunter

On Mon, Nov 04, 2024, Xiaoyao Li wrote:
> On 11/2/2024 2:50 AM, Sean Christopherson wrote:
> > Hide KVM's pt_mode module param behind CONFIG_BROKEN, i.e. disable support
> > for virtualizing Intel PT via guest/host mode unless BROKEN=y.  There are
> > myriad bugs in the implementation, some of which are fatal to the guest,
> > and others which put the stability and health of the host at risk.
> > 
> > For guest fatalities, the most glaring issue is that KVM fails to ensure
> > tracing is disabled, and *stays* disabled prior to VM-Enter, which is
> > necessary as hardware disallows loading (the guest's) RTIT_CTL if tracing
> > is enabled (enforced via a VMX consistency check).  Per the SDM:
> > 
> >    If the logical processor is operating with Intel PT enabled (if
> >    IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the "load
> >    IA32_RTIT_CTL" VM-entry control must be 0.
> > 
> > On the host side, KVM doesn't validate the guest CPUID configuration
> > provided by userspace, and even worse, uses the guest configuration to
> > decide what MSRs to save/load at VM-Enter and VM-Exit.  E.g. configuring
> > guest CPUID to enumerate more address ranges than are supported in hardware
> > will result in KVM trying to passthrough, save, and load non-existent MSRs,
> > which generates a variety of WARNs, ToPA ERRORs in the host, a potential
> > deadlock, etc.
> > 
> > Fixes: f99e3daf94ff ("KVM: x86: Add Intel PT virtualization work mode")
> > Cc: stable@vger.kernel.org
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   arch/x86/kvm/vmx/vmx.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 6ed801ffe33f..087504fb1589 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -217,9 +217,11 @@ module_param(ple_window_shrink, uint, 0444);
> >   static unsigned int ple_window_max        = KVM_VMX_DEFAULT_PLE_WINDOW_MAX;
> >   module_param(ple_window_max, uint, 0444);
> > -/* Default is SYSTEM mode, 1 for host-guest mode */
> > +/* Default is SYSTEM mode, 1 for host-guest mode (which is BROKEN) */
> >   int __read_mostly pt_mode = PT_MODE_SYSTEM;
> > +#ifdef CONFIG_BROKEN
> >   module_param(pt_mode, int, S_IRUGO);
> > +#endif
> 
> I like the patch, but I didn't find any other usercase of CONFIG_BROKEN in
> current Linux.

Ya, BROKEN is typically used directly in Kconfigs, e.g. "depends on BROKEN".  But
I can't think of any reason using it in this way would be problematic.

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

* Re: [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN
  2024-11-04  8:23   ` Adrian Hunter
@ 2024-11-04 23:52     ` Sean Christopherson
  0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-11-04 23:52 UTC (permalink / raw)
  To: Adrian Hunter; +Cc: Paolo Bonzini, kvm, linux-kernel

On Mon, Nov 04, 2024, Adrian Hunter wrote:
> On 1/11/24 20:50, Sean Christopherson wrote:
> > Hide KVM's pt_mode module param behind CONFIG_BROKEN, i.e. disable support
> > for virtualizing Intel PT via guest/host mode unless BROKEN=y.  There are
> > myriad bugs in the implementation, some of which are fatal to the guest,
> > and others which put the stability and health of the host at risk.
> > 
> > For guest fatalities, the most glaring issue is that KVM fails to ensure
> > tracing is disabled, and *stays* disabled prior to VM-Enter, which is
> > necessary as hardware disallows loading (the guest's) RTIT_CTL if tracing
> > is enabled (enforced via a VMX consistency check).  Per the SDM:
> > 
> >   If the logical processor is operating with Intel PT enabled (if
> >   IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the "load
> >   IA32_RTIT_CTL" VM-entry control must be 0.
> > 
> > On the host side, KVM doesn't validate the guest CPUID configuration
> > provided by userspace, and even worse, uses the guest configuration to
> > decide what MSRs to save/load at VM-Enter and VM-Exit.  E.g. configuring
> > guest CPUID to enumerate more address ranges than are supported in hardware
> > will result in KVM trying to passthrough, save, and load non-existent MSRs,
> > which generates a variety of WARNs, ToPA ERRORs in the host, a potential
> > deadlock, etc.
> > 
> > Fixes: f99e3daf94ff ("KVM: x86: Add Intel PT virtualization work mode")
> > Cc: stable@vger.kernel.org
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 6ed801ffe33f..087504fb1589 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -217,9 +217,11 @@ module_param(ple_window_shrink, uint, 0444);
> >  static unsigned int ple_window_max        = KVM_VMX_DEFAULT_PLE_WINDOW_MAX;
> >  module_param(ple_window_max, uint, 0444);
> >  
> > -/* Default is SYSTEM mode, 1 for host-guest mode */
> > +/* Default is SYSTEM mode, 1 for host-guest mode (which is BROKEN) */
> >  int __read_mostly pt_mode = PT_MODE_SYSTEM;
> > +#ifdef CONFIG_BROKEN
> >  module_param(pt_mode, int, S_IRUGO);
> > +#endif
> 
> Side effects are:
> 1. If pt_mode is passed via modprobe, there will be a warning in kernel messages:
> 	kvm_intel: unknown parameter 'pt_mode' ignored

This is more of a feature in this case, as it's a non-fatal way of alerting the
user that trying to enable PT virtualization won't work.

> 2. The sysfs module parameter file pt_mode will be gone:
> 	# cat /sys/module/kvm_intel/parameters/pt_mode
> 	cat: /sys/module/kvm_intel/parameters/pt_mode: No such file or directory

Hrm, this could be slightly more problematic, e.g. if userspace were asserting on
the state of the parameter.  But AFAIK, module params aren't considered ABI.

Paolo, any thoughts on how best to handle this?

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

* Re: [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN
  2024-11-04 22:46     ` Sean Christopherson
@ 2024-11-05 13:34       ` Xiaoyao Li
  0 siblings, 0 replies; 11+ messages in thread
From: Xiaoyao Li @ 2024-11-05 13:34 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: Paolo Bonzini, kvm, linux-kernel, Adrian Hunter

On 11/5/2024 6:46 AM, Sean Christopherson wrote:
> On Mon, Nov 04, 2024, Xiaoyao Li wrote:
>> On 11/2/2024 2:50 AM, Sean Christopherson wrote:
>>> Hide KVM's pt_mode module param behind CONFIG_BROKEN, i.e. disable support
>>> for virtualizing Intel PT via guest/host mode unless BROKEN=y.  There are
>>> myriad bugs in the implementation, some of which are fatal to the guest,
>>> and others which put the stability and health of the host at risk.
>>>
>>> For guest fatalities, the most glaring issue is that KVM fails to ensure
>>> tracing is disabled, and *stays* disabled prior to VM-Enter, which is
>>> necessary as hardware disallows loading (the guest's) RTIT_CTL if tracing
>>> is enabled (enforced via a VMX consistency check).  Per the SDM:
>>>
>>>     If the logical processor is operating with Intel PT enabled (if
>>>     IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the "load
>>>     IA32_RTIT_CTL" VM-entry control must be 0.
>>>
>>> On the host side, KVM doesn't validate the guest CPUID configuration
>>> provided by userspace, and even worse, uses the guest configuration to
>>> decide what MSRs to save/load at VM-Enter and VM-Exit.  E.g. configuring
>>> guest CPUID to enumerate more address ranges than are supported in hardware
>>> will result in KVM trying to passthrough, save, and load non-existent MSRs,
>>> which generates a variety of WARNs, ToPA ERRORs in the host, a potential
>>> deadlock, etc.
>>>
>>> Fixes: f99e3daf94ff ("KVM: x86: Add Intel PT virtualization work mode")
>>> Cc: stable@vger.kernel.org
>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>    arch/x86/kvm/vmx/vmx.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>> index 6ed801ffe33f..087504fb1589 100644
>>> --- a/arch/x86/kvm/vmx/vmx.c
>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>> @@ -217,9 +217,11 @@ module_param(ple_window_shrink, uint, 0444);
>>>    static unsigned int ple_window_max        = KVM_VMX_DEFAULT_PLE_WINDOW_MAX;
>>>    module_param(ple_window_max, uint, 0444);
>>> -/* Default is SYSTEM mode, 1 for host-guest mode */
>>> +/* Default is SYSTEM mode, 1 for host-guest mode (which is BROKEN) */
>>>    int __read_mostly pt_mode = PT_MODE_SYSTEM;
>>> +#ifdef CONFIG_BROKEN
>>>    module_param(pt_mode, int, S_IRUGO);
>>> +#endif
>>
>> I like the patch, but I didn't find any other usercase of CONFIG_BROKEN in
>> current Linux.
> 
> Ya, BROKEN is typically used directly in Kconfigs, e.g. "depends on BROKEN".  But
> I can't think of any reason using it in this way would be problematic.

I see. Thanks for the information!

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>


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

* Re: [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN
  2024-11-01 18:50 ` [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN Sean Christopherson
  2024-11-04  1:56   ` Xiaoyao Li
  2024-11-04  8:23   ` Adrian Hunter
@ 2024-11-08  9:07   ` Paolo Bonzini
  2 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2024-11-08  9:07 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, linux-kernel, Adrian Hunter

On 11/1/24 19:50, Sean Christopherson wrote:
> Hide KVM's pt_mode module param behind CONFIG_BROKEN, i.e. disable support
> for virtualizing Intel PT via guest/host mode unless BROKEN=y.  There are
> myriad bugs in the implementation, some of which are fatal to the guest,
> and others which put the stability and health of the host at risk.
> 
> For guest fatalities, the most glaring issue is that KVM fails to ensure
> tracing is disabled, and *stays* disabled prior to VM-Enter, which is
> necessary as hardware disallows loading (the guest's) RTIT_CTL if tracing
> is enabled (enforced via a VMX consistency check).  Per the SDM:
> 
>    If the logical processor is operating with Intel PT enabled (if
>    IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the "load
>    IA32_RTIT_CTL" VM-entry control must be 0.
> 
> On the host side, KVM doesn't validate the guest CPUID configuration
> provided by userspace, and even worse, uses the guest configuration to
> decide what MSRs to save/load at VM-Enter and VM-Exit.  E.g. configuring
> guest CPUID to enumerate more address ranges than are supported in hardware
> will result in KVM trying to passthrough, save, and load non-existent MSRs,
> which generates a variety of WARNs, ToPA ERRORs in the host, a potential
> deadlock, etc.
> 
> Fixes: f99e3daf94ff ("KVM: x86: Add Intel PT virtualization work mode")
> Cc: stable@vger.kernel.org
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/vmx/vmx.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6ed801ffe33f..087504fb1589 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -217,9 +217,11 @@ module_param(ple_window_shrink, uint, 0444);
>   static unsigned int ple_window_max        = KVM_VMX_DEFAULT_PLE_WINDOW_MAX;
>   module_param(ple_window_max, uint, 0444);
>   
> -/* Default is SYSTEM mode, 1 for host-guest mode */
> +/* Default is SYSTEM mode, 1 for host-guest mode (which is BROKEN) */
>   int __read_mostly pt_mode = PT_MODE_SYSTEM;
> +#ifdef CONFIG_BROKEN
>   module_param(pt_mode, int, S_IRUGO);
> +#endif
>   
>   struct x86_pmu_lbr __ro_after_init vmx_lbr_caps;
>   

Applied, thanks.

Paolo


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

* Re: [PATCH 0/2] KVM: VMX: Mark Intel PT virtualization as BROKEN
  2024-11-01 18:50 [PATCH 0/2] KVM: VMX: Mark Intel PT virtualization as BROKEN Sean Christopherson
  2024-11-01 18:50 ` [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN Sean Christopherson
  2024-11-01 18:50 ` [PATCH 2/2] KVM: VMX: Allow toggling bits in MSR_IA32_RTIT_CTL when enable bit is cleared Sean Christopherson
@ 2024-12-19  2:41 ` Sean Christopherson
  2 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-12-19  2:41 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Adrian Hunter

On Fri, 01 Nov 2024 11:50:29 -0700, Sean Christopherson wrote:
> Hide Intel PT virtualization behind BROKEN, as it has multiple fatal
> flaws, several which put the host at risk, and several of which are far
> too invasive to backport to stable trees.
> 
> I included one of the easier fixes from Adrian to help show just how
> broken PT virtualization is, e.g. to illustrate the apparent lack of usage.
> 
> [...]

Applied patch 2 to kvm-x86 vmx (patch 1 already went in).

[1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN
      (no commit info)
[2/2] KVM: VMX: Allow toggling bits in MSR_IA32_RTIT_CTL when enable bit is cleared
      https://github.com/kvm-x86/linux/commit/591ff4670c7b

--
https://github.com/kvm-x86/linux/tree/next

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

end of thread, other threads:[~2024-12-19  2:44 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-01 18:50 [PATCH 0/2] KVM: VMX: Mark Intel PT virtualization as BROKEN Sean Christopherson
2024-11-01 18:50 ` [PATCH 1/2] KVM: VMX: Bury Intel PT virtualization (guest/host mode) behind CONFIG_BROKEN Sean Christopherson
2024-11-04  1:56   ` Xiaoyao Li
2024-11-04 22:46     ` Sean Christopherson
2024-11-05 13:34       ` Xiaoyao Li
2024-11-04  8:23   ` Adrian Hunter
2024-11-04 23:52     ` Sean Christopherson
2024-11-08  9:07   ` Paolo Bonzini
2024-11-01 18:50 ` [PATCH 2/2] KVM: VMX: Allow toggling bits in MSR_IA32_RTIT_CTL when enable bit is cleared Sean Christopherson
2024-11-04  1:57   ` Xiaoyao Li
2024-12-19  2:41 ` [PATCH 0/2] KVM: VMX: Mark Intel PT virtualization as BROKEN Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox