| Message ID | 20200201185218.24473-30-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Introduce KVM cpu caps | expand |
Sean Christopherson <sean.j.christopherson@intel.com> writes: > Add WARNs in the low level __cpuid_entry_get_reg() to assert that the > function and index of the CPUID entry and reverse CPUID entry match. > Wrap the WARNs in a new Kconfig, KVM_CPUID_AUDIT, as the checks add > almost no value in a production environment, i.e. will only detect > blatant KVM bugs and fatal hardware errors. Add a Kconfig instead of > simply wrapping the WARNs with an off-by-default #ifdef so that syzbot > and other automated testing can enable the auditing. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/Kconfig | 10 ++++++++++ > arch/x86/kvm/cpuid.h | 5 +++++ > 2 files changed, 15 insertions(+) > > diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig > index 840e12583b85..bbbc3258358e 100644 > --- a/arch/x86/kvm/Kconfig > +++ b/arch/x86/kvm/Kconfig > @@ -96,6 +96,16 @@ config KVM_MMU_AUDIT > This option adds a R/W kVM module parameter 'mmu_audit', which allows > auditing of KVM MMU events at runtime. > > +config KVM_CPUID_AUDIT > + bool "Audit KVM reverse CPUID lookups" > + depends on KVM > + help > + This option enables runtime checking of reverse CPUID lookups in KVM > + to verify the function and index of the referenced X86_FEATURE_* match > + the function and index of the CPUID entry being accessed. > + > + If unsure, say N. > + > # OK, it's a little counter-intuitive to do this, but it puts it neatly under > # the virtualization menu. > source "drivers/vhost/Kconfig" > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > index 51f19eade5a0..41ff94a7d3e0 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -98,6 +98,11 @@ static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature) > static __always_inline u32 *__cpuid_entry_get_reg(struct kvm_cpuid_entry2 *entry, > const struct cpuid_reg *cpuid) > { > +#ifdef CONFIG_KVM_CPUID_AUDIT > + WARN_ON_ONCE(entry->function != cpuid->function); > + WARN_ON_ONCE(entry->index != cpuid->index); > +#endif > + > switch (cpuid->reg) { > case CPUID_EAX: > return &entry->eax; Honestly, I was thinking we should BUG_ON() and even in production builds but not everyone around is so rebellious I guess, so Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
On Mon, Feb 24, 2020 at 02:54:38PM +0100, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > > > Add WARNs in the low level __cpuid_entry_get_reg() to assert that the > > function and index of the CPUID entry and reverse CPUID entry match. > > Wrap the WARNs in a new Kconfig, KVM_CPUID_AUDIT, as the checks add > > almost no value in a production environment, i.e. will only detect > > blatant KVM bugs and fatal hardware errors. Add a Kconfig instead of > > simply wrapping the WARNs with an off-by-default #ifdef so that syzbot > > and other automated testing can enable the auditing. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kvm/Kconfig | 10 ++++++++++ > > arch/x86/kvm/cpuid.h | 5 +++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig > > index 840e12583b85..bbbc3258358e 100644 > > --- a/arch/x86/kvm/Kconfig > > +++ b/arch/x86/kvm/Kconfig > > @@ -96,6 +96,16 @@ config KVM_MMU_AUDIT > > This option adds a R/W kVM module parameter 'mmu_audit', which allows > > auditing of KVM MMU events at runtime. > > > > +config KVM_CPUID_AUDIT > > + bool "Audit KVM reverse CPUID lookups" > > + depends on KVM > > + help > > + This option enables runtime checking of reverse CPUID lookups in KVM > > + to verify the function and index of the referenced X86_FEATURE_* match > > + the function and index of the CPUID entry being accessed. > > + > > + If unsure, say N. > > + > > # OK, it's a little counter-intuitive to do this, but it puts it neatly under > > # the virtualization menu. > > source "drivers/vhost/Kconfig" > > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > > index 51f19eade5a0..41ff94a7d3e0 100644 > > --- a/arch/x86/kvm/cpuid.h > > +++ b/arch/x86/kvm/cpuid.h > > @@ -98,6 +98,11 @@ static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature) > > static __always_inline u32 *__cpuid_entry_get_reg(struct kvm_cpuid_entry2 *entry, > > const struct cpuid_reg *cpuid) > > { > > +#ifdef CONFIG_KVM_CPUID_AUDIT > > + WARN_ON_ONCE(entry->function != cpuid->function); > > + WARN_ON_ONCE(entry->index != cpuid->index); > > +#endif > > + > > switch (cpuid->reg) { > > case CPUID_EAX: > > return &entry->eax; > > Honestly, I was thinking we should BUG_ON() and even in production builds > but not everyone around is so rebellious I guess, so LOL. It's a waste of cycles for something that will "never" be hit, i.e. we _really_ dropped the ball if a bug of this natures makes it into a kernel release. > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > -- > Vitaly >
On 24/02/20 14:54, Vitaly Kuznetsov wrote: >> --- a/arch/x86/kvm/cpuid.h >> +++ b/arch/x86/kvm/cpuid.h >> @@ -98,6 +98,11 @@ static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature) >> static __always_inline u32 *__cpuid_entry_get_reg(struct kvm_cpuid_entry2 *entry, >> const struct cpuid_reg *cpuid) >> { >> +#ifdef CONFIG_KVM_CPUID_AUDIT >> + WARN_ON_ONCE(entry->function != cpuid->function); >> + WARN_ON_ONCE(entry->index != cpuid->index); >> +#endif >> + >> switch (cpuid->reg) { >> case CPUID_EAX: >> return &entry->eax; > Honestly, I was thinking we should BUG_ON() and even in production builds > but not everyone around is so rebellious I guess, so BUG_ON is too much, but I agree the cost is so small that unconditional WARN_ON makes sense. Paolo
On 24/02/20 23:46, Sean Christopherson wrote: >>> static __always_inline u32 *__cpuid_entry_get_reg(struct kvm_cpuid_entry2 *entry, >>> const struct cpuid_reg *cpuid) >>> { >>> +#ifdef CONFIG_KVM_CPUID_AUDIT >>> + WARN_ON_ONCE(entry->function != cpuid->function); >>> + WARN_ON_ONCE(entry->index != cpuid->index); >>> +#endif >>> + >>> switch (cpuid->reg) { >>> case CPUID_EAX: >>> return &entry->eax; >> >> Honestly, I was thinking we should BUG_ON() and even in production builds >> but not everyone around is so rebellious I guess, so > > LOL. It's a waste of cycles for something that will "never" be hit, i.e. > we _really_ dropped the ball if a bug of this natures makes it into a > kernel release. There are quite a few WARN_ONs like that already. I'd say each non-constant-folded call to __cpuid_enty_get_reg is a waste of cycles, if you're counting them. :) Paolo
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig index 840e12583b85..bbbc3258358e 100644 --- a/arch/x86/kvm/Kconfig +++ b/arch/x86/kvm/Kconfig @@ -96,6 +96,16 @@ config KVM_MMU_AUDIT This option adds a R/W kVM module parameter 'mmu_audit', which allows auditing of KVM MMU events at runtime. +config KVM_CPUID_AUDIT + bool "Audit KVM reverse CPUID lookups" + depends on KVM + help + This option enables runtime checking of reverse CPUID lookups in KVM + to verify the function and index of the referenced X86_FEATURE_* match + the function and index of the CPUID entry being accessed. + + If unsure, say N. + # OK, it's a little counter-intuitive to do this, but it puts it neatly under # the virtualization menu. source "drivers/vhost/Kconfig" diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h index 51f19eade5a0..41ff94a7d3e0 100644 --- a/arch/x86/kvm/cpuid.h +++ b/arch/x86/kvm/cpuid.h @@ -98,6 +98,11 @@ static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature) static __always_inline u32 *__cpuid_entry_get_reg(struct kvm_cpuid_entry2 *entry, const struct cpuid_reg *cpuid) { +#ifdef CONFIG_KVM_CPUID_AUDIT + WARN_ON_ONCE(entry->function != cpuid->function); + WARN_ON_ONCE(entry->index != cpuid->index); +#endif + switch (cpuid->reg) { case CPUID_EAX: return &entry->eax;
Add WARNs in the low level __cpuid_entry_get_reg() to assert that the function and index of the CPUID entry and reverse CPUID entry match. Wrap the WARNs in a new Kconfig, KVM_CPUID_AUDIT, as the checks add almost no value in a production environment, i.e. will only detect blatant KVM bugs and fatal hardware errors. Add a Kconfig instead of simply wrapping the WARNs with an off-by-default #ifdef so that syzbot and other automated testing can enable the auditing. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/Kconfig | 10 ++++++++++ arch/x86/kvm/cpuid.h | 5 +++++ 2 files changed, 15 insertions(+)