* [PATCH v2] KVM: x86/pt: Ignore all unknown Intel PT capabilities @ 2022-01-10 3:47 Like Xu 2022-01-11 0:57 ` Sean Christopherson 0 siblings, 1 reply; 6+ messages in thread From: Like Xu @ 2022-01-10 3:47 UTC (permalink / raw) To: Paolo Bonzini Cc: Sean Christopherson, Jim Mattson, Wanpeng Li, Vitaly Kuznetsov, Joerg Roedel, x86, kvm, linux-kernel From: Like Xu <likexu@tencent.com> Some of the new Intel PT capabilities (e.g. SDM Vol3, 32.2.4 Event Tracing, it exposes details about the asynchronous events, when they are generated, and when their corresponding software event handler completes execution) cannot be safely and fully emulated by the KVM, especially emulating the simultaneous writing of guest PT packets generated by the KVM to the guest PT buffer. For KVM, it's better to advertise currently supported features based on the "static struct pt_cap_desc" implemented in the host PT driver and ignore _all_ unknown features before they have been investigated one by one and supported in a safe manner, leaving the rest as system-wide-only tracing capabilities. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Like Xu <likexu@tencent.com> --- v1 -> v2 Changelog: - Be safe and ignore _all_ unknown capabilities. (Paolo) Previous: https://lore.kernel.org/kvm/20220106085533.84356-1-likexu@tencent.com/ arch/x86/kvm/cpuid.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 0b920e12bb6d..439b93359848 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -901,6 +901,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) break; } + /* It's better to be safe and ignore _all_ unknown capabilities. */ + entry->ebx &= GENMASK(5, 0); for (i = 1, max_idx = entry->eax; i <= max_idx; ++i) { if (!do_host_cpuid(array, function, i)) goto out; -- 2.33.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: x86/pt: Ignore all unknown Intel PT capabilities 2022-01-10 3:47 [PATCH v2] KVM: x86/pt: Ignore all unknown Intel PT capabilities Like Xu @ 2022-01-11 0:57 ` Sean Christopherson 2022-01-11 4:20 ` Like Xu 0 siblings, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2022-01-11 0:57 UTC (permalink / raw) To: Like Xu Cc: Paolo Bonzini, Jim Mattson, Wanpeng Li, Vitaly Kuznetsov, Joerg Roedel, x86, kvm, linux-kernel On Mon, Jan 10, 2022, Like Xu wrote: > From: Like Xu <likexu@tencent.com> > > Some of the new Intel PT capabilities (e.g. SDM Vol3, 32.2.4 Event > Tracing, it exposes details about the asynchronous events, when they are > generated, and when their corresponding software event handler completes > execution) cannot be safely and fully emulated by the KVM, especially > emulating the simultaneous writing of guest PT packets generated by > the KVM to the guest PT buffer. > > For KVM, it's better to advertise currently supported features based on > the "static struct pt_cap_desc" implemented in the host PT driver and > ignore _all_ unknown features before they have been investigated one by > one and supported in a safe manner, leaving the rest as system-wide-only > tracing capabilities. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Like Xu <likexu@tencent.com> > --- > v1 -> v2 Changelog: > - Be safe and ignore _all_ unknown capabilities. (Paolo) > > Previous: > https://lore.kernel.org/kvm/20220106085533.84356-1-likexu@tencent.com/ > > arch/x86/kvm/cpuid.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0b920e12bb6d..439b93359848 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -901,6 +901,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) > break; > } > > + /* It's better to be safe and ignore _all_ unknown capabilities. */ No need to justify why unknown capabilities are hidden as that's very much (supposed to be) standard KVM behavior. > + entry->ebx &= GENMASK(5, 0); Please add a #define somewhere so that this is self-documenting, e.g. see KVM_SUPPORTED_XCR0. And why just EBX? ECX appears to enumerate features too, and EDX is presumably reserved to enumerate yet more features when EBX/ECX run out of bits. And is there any possibility of a malicious user/guest using features to cause problems in the host? I.e. does KVM need to enforce that the guest can't enable any unsupported features? > for (i = 1, max_idx = entry->eax; i <= max_idx; ++i) { > if (!do_host_cpuid(array, function, i)) > goto out; > -- > 2.33.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: x86/pt: Ignore all unknown Intel PT capabilities 2022-01-11 0:57 ` Sean Christopherson @ 2022-01-11 4:20 ` Like Xu 2022-01-11 6:24 ` Like Xu 2022-01-11 7:59 ` Xiaoyao Li 0 siblings, 2 replies; 6+ messages in thread From: Like Xu @ 2022-01-11 4:20 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, Jim Mattson, Wanpeng Li, Vitaly Kuznetsov, Joerg Roedel, x86, kvm, linux-kernel On 11/1/2022 8:57 am, Sean Christopherson wrote: > On Mon, Jan 10, 2022, Like Xu wrote: >> From: Like Xu <likexu@tencent.com> >> >> Some of the new Intel PT capabilities (e.g. SDM Vol3, 32.2.4 Event >> Tracing, it exposes details about the asynchronous events, when they are >> generated, and when their corresponding software event handler completes >> execution) cannot be safely and fully emulated by the KVM, especially >> emulating the simultaneous writing of guest PT packets generated by >> the KVM to the guest PT buffer. >> >> For KVM, it's better to advertise currently supported features based on >> the "static struct pt_cap_desc" implemented in the host PT driver and >> ignore _all_ unknown features before they have been investigated one by >> one and supported in a safe manner, leaving the rest as system-wide-only >> tracing capabilities. >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Like Xu <likexu@tencent.com> >> --- >> v1 -> v2 Changelog: >> - Be safe and ignore _all_ unknown capabilities. (Paolo) >> >> Previous: >> https://lore.kernel.org/kvm/20220106085533.84356-1-likexu@tencent.com/ >> >> arch/x86/kvm/cpuid.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >> index 0b920e12bb6d..439b93359848 100644 >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -901,6 +901,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) >> break; >> } >> >> + /* It's better to be safe and ignore _all_ unknown capabilities. */ > > No need to justify why unknown capabilities are hidden as that's very much (supposed > to be) standard KVM behavior. > >> + entry->ebx &= GENMASK(5, 0); > > Please add a #define somewhere so that this is self-documenting, e.g. see > KVM_SUPPORTED_XCR0. How about we define this macro in the <asm/intel_pt.h> so that the next PT capability enabler can update the mask with minimal effort, considering that many pure kernel developers don't care about KVM code ? > > And why just EBX? ECX appears to enumerate features too, and EDX is presumably > reserved to enumerate yet more features when EBX/ECX run out of bits. Yes, how about this version: diff --git a/arch/x86/include/asm/intel_pt.h b/arch/x86/include/asm/intel_pt.h index ebe8d2ea44fe..da94d0eeb9df 100644 --- a/arch/x86/include/asm/intel_pt.h +++ b/arch/x86/include/asm/intel_pt.h @@ -24,6 +24,12 @@ enum pt_capabilities { PT_CAP_psb_periods, }; +#define GUEST_SUPPORTED_CPUID_14_EBX \ + (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(4) | BIT(5)) + +#define GUEST_SUPPORTED_CPUID_14_ECX \ + (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(31)) + #if defined(CONFIG_PERF_EVENTS) && defined(CONFIG_CPU_SUP_INTEL) void cpu_emergency_stop_pt(void); extern u32 intel_pt_validate_hw_cap(enum pt_capabilities cap); diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 0b920e12bb6d..be8c9170f98e 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -19,6 +19,7 @@ #include <asm/user.h> #include <asm/fpu/xstate.h> #include <asm/sgx.h> +#include <asm/intel_pt.h> #include "cpuid.h" #include "lapic.h" #include "mmu.h" @@ -900,7 +901,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) entry->eax = entry->ebx = entry->ecx = entry->edx = 0; break; } - + entry->eax = min(entry->eax, 1u); + entry->ebx &= GUEST_SUPPORTED_CPUID_14_EBX; + entry->ecx &= GUEST_SUPPORTED_CPUID_14_ECX; + entry->edx = 0; for (i = 1, max_idx = entry->eax; i <= max_idx; ++i) { if (!do_host_cpuid(array, function, i)) goto out; > > And is there any possibility of a malicious user/guest using features to cause > problems in the host? I.e. does KVM need to enforce that the guest can't enable > any unsupported features? If a user space is set up with features not supported by KVM, it owns the risk itself. AFAI, the guest Intel PT introduces a great attack interface for the host and we only use the guest supported PT features in a highly trusted environment. I agree that more uncertainty and fixes can be triggered in the security motive, not expecting too much from this patch. :D > >> for (i = 1, max_idx = entry->eax; i <= max_idx; ++i) { >> if (!do_host_cpuid(array, function, i)) >> goto out; >> -- >> 2.33.1 >> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: x86/pt: Ignore all unknown Intel PT capabilities 2022-01-11 4:20 ` Like Xu @ 2022-01-11 6:24 ` Like Xu 2022-01-11 7:59 ` Xiaoyao Li 1 sibling, 0 replies; 6+ messages in thread From: Like Xu @ 2022-01-11 6:24 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini Cc: Jim Mattson, Wanpeng Li, Vitaly Kuznetsov, Joerg Roedel, x86, kvm, linux-kernel On 11/1/2022 12:20 pm, Like Xu wrote: >> And is there any possibility of a malicious user/guest using features to cause >> problems in the host? I.e. does KVM need to enforce that the guest can't enable >> any unsupported features? > > If a user space is set up with features not supported by KVM, it owns the risk > itself. I seem to have misunderstood it. KVM should prevent and stop any malicious guest from destroying other parts on the host, is this the right direction ? > > AFAI, the guest Intel PT introduces a great attack interface for the host and > we only use the guest supported PT features in a highly trusted environment. > > I agree that more uncertainty and fixes can be triggered in the security motive, > not expecting too much from this patch. :D ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: x86/pt: Ignore all unknown Intel PT capabilities 2022-01-11 4:20 ` Like Xu 2022-01-11 6:24 ` Like Xu @ 2022-01-11 7:59 ` Xiaoyao Li 2022-01-11 8:15 ` Like Xu 1 sibling, 1 reply; 6+ messages in thread From: Xiaoyao Li @ 2022-01-11 7:59 UTC (permalink / raw) To: Like Xu, Sean Christopherson Cc: Paolo Bonzini, Jim Mattson, Wanpeng Li, Vitaly Kuznetsov, Joerg Roedel, x86, kvm, linux-kernel On 1/11/2022 12:20 PM, Like Xu wrote: > On 11/1/2022 8:57 am, Sean Christopherson wrote: >> On Mon, Jan 10, 2022, Like Xu wrote: >>> From: Like Xu <likexu@tencent.com> >>> >>> Some of the new Intel PT capabilities (e.g. SDM Vol3, 32.2.4 Event >>> Tracing, it exposes details about the asynchronous events, when they are >>> generated, and when their corresponding software event handler completes >>> execution) cannot be safely and fully emulated by the KVM, especially >>> emulating the simultaneous writing of guest PT packets generated by >>> the KVM to the guest PT buffer. >>> >>> For KVM, it's better to advertise currently supported features based on >>> the "static struct pt_cap_desc" implemented in the host PT driver and >>> ignore _all_ unknown features before they have been investigated one by >>> one and supported in a safe manner, leaving the rest as system-wide-only >>> tracing capabilities. >>> >>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Like Xu <likexu@tencent.com> >>> --- >>> v1 -> v2 Changelog: >>> - Be safe and ignore _all_ unknown capabilities. (Paolo) >>> >>> Previous: >>> https://lore.kernel.org/kvm/20220106085533.84356-1-likexu@tencent.com/ >>> >>> arch/x86/kvm/cpuid.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >>> index 0b920e12bb6d..439b93359848 100644 >>> --- a/arch/x86/kvm/cpuid.c >>> +++ b/arch/x86/kvm/cpuid.c >>> @@ -901,6 +901,8 @@ static inline int __do_cpuid_func(struct >>> kvm_cpuid_array *array, u32 function) >>> break; >>> } >>> + /* It's better to be safe and ignore _all_ unknown >>> capabilities. */ >> >> No need to justify why unknown capabilities are hidden as that's very >> much (supposed >> to be) standard KVM behavior. >> >>> + entry->ebx &= GENMASK(5, 0); >> >> Please add a #define somewhere so that this is self-documenting, e.g. see >> KVM_SUPPORTED_XCR0. > > How about we define this macro in the <asm/intel_pt.h> so that the next > PT capability > enabler can update the mask with minimal effort, considering that many > pure kernel > developers don't care about KVM code ? > >> >> And why just EBX? ECX appears to enumerate features too, and EDX is >> presumably >> reserved to enumerate yet more features when EBX/ECX run out of bits. > > Yes, how about this version: > > diff --git a/arch/x86/include/asm/intel_pt.h > b/arch/x86/include/asm/intel_pt.h > index ebe8d2ea44fe..da94d0eeb9df 100644 > --- a/arch/x86/include/asm/intel_pt.h > +++ b/arch/x86/include/asm/intel_pt.h > @@ -24,6 +24,12 @@ enum pt_capabilities { > PT_CAP_psb_periods, > }; > > +#define GUEST_SUPPORTED_CPUID_14_EBX \ > + (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(4) | BIT(5)) > + > +#define GUEST_SUPPORTED_CPUID_14_ECX \ > + (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(31)) > + I doubt BIT(3) of CPUID_14_ECX can be exposed to guest directly. It means "output to Trace Transport Subsystem Supported". If I understand correctly, it at least needs passthrough of the said Transport Subsystem or emulation of it. > #if defined(CONFIG_PERF_EVENTS) && defined(CONFIG_CPU_SUP_INTEL) > void cpu_emergency_stop_pt(void); > extern u32 intel_pt_validate_hw_cap(enum pt_capabilities cap); > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0b920e12bb6d..be8c9170f98e 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -19,6 +19,7 @@ > #include <asm/user.h> > #include <asm/fpu/xstate.h> > #include <asm/sgx.h> > +#include <asm/intel_pt.h> > #include "cpuid.h" > #include "lapic.h" > #include "mmu.h" > @@ -900,7 +901,10 @@ static inline int __do_cpuid_func(struct > kvm_cpuid_array *array, u32 function) > entry->eax = entry->ebx = entry->ecx = entry->edx = 0; > break; > } > - > + entry->eax = min(entry->eax, 1u); > + entry->ebx &= GUEST_SUPPORTED_CPUID_14_EBX; > + entry->ecx &= GUEST_SUPPORTED_CPUID_14_ECX; > + entry->edx = 0; > for (i = 1, max_idx = entry->eax; i <= max_idx; ++i) { > if (!do_host_cpuid(array, function, i)) > goto out; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: x86/pt: Ignore all unknown Intel PT capabilities 2022-01-11 7:59 ` Xiaoyao Li @ 2022-01-11 8:15 ` Like Xu 0 siblings, 0 replies; 6+ messages in thread From: Like Xu @ 2022-01-11 8:15 UTC (permalink / raw) To: Xiaoyao Li Cc: Paolo Bonzini, Jim Mattson, Wanpeng Li, Vitaly Kuznetsov, Joerg Roedel, x86, kvm, linux-kernel, Sean Christopherson On 11/1/2022 3:59 pm, Xiaoyao Li wrote: >> >> +#define GUEST_SUPPORTED_CPUID_14_EBX \ >> + (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(4) | BIT(5)) >> + >> +#define GUEST_SUPPORTED_CPUID_14_ECX \ >> + (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(31)) >> + > > I doubt BIT(3) of CPUID_14_ECX can be exposed to guest directly. > > It means "output to Trace Transport Subsystem Supported". If I understand > correctly, it at least needs passthrough of the said Transport Subsystem or > emulation of it. I'm not surprised that we can route Intel Guest PT output to a platform-specific trace endpoint (e.g., physical or emulated JTAG) as an MMIO debug port. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-11 8:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-10 3:47 [PATCH v2] KVM: x86/pt: Ignore all unknown Intel PT capabilities Like Xu 2022-01-11 0:57 ` Sean Christopherson 2022-01-11 4:20 ` Like Xu 2022-01-11 6:24 ` Like Xu 2022-01-11 7:59 ` Xiaoyao Li 2022-01-11 8:15 ` Like Xu
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).