Re: [PATCH 07/14] KVM: x86: SVM: add warning for CVE-2021-3656

From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Maxim Levitsky <mlevitsk@redhat.com>,
	kvm@vger.kernel.org, Vitaly Kuznetsov <vkuznets@redhat.com>,
	Borislav Petkov <bp@alien8.de>, Bandan Das <bsd@redhat.com>,
	open list <linux-kernel@vger.kernel.org>,
	Joerg Roedel <joro@8bytes.org>, Ingo Molnar <mingo@redhat.com>,
	Wei Huang <wei.huang2@amd.com>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Jim Mattson <jmattson@google.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
	<x86@kernel.org>, Shuah Khan <shuah@kernel.org>,
	Wanpeng Li <wanpengli@tencent.com>
Subject: Re: [PATCH 07/14] KVM: x86: SVM: add warning for CVE-2021-3656
Date: Tue, 12 Oct 2021 00:21:45 +0000	[thread overview]
Message-ID: <YWTVGaX4V1eR6k0k@google.com> (raw)
In-Reply-To: <f0c0e659-23a8-59ab-edf8-5b380d723493@redhat.com>

On Thu, Sep 23, 2021, Paolo Bonzini wrote:
> On 14/09/21 17:48, Maxim Levitsky wrote:
> > Just in case, add a warning ensuring that on guest entry,
> > either both VMLOAD and VMSAVE intercept is enabled or
> > vVMLOAD/VMSAVE is enabled.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >   arch/x86/kvm/svm/svm.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 861ac9f74331..deeebd05f682 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -3784,6 +3784,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
> >   	WARN_ON_ONCE(kvm_apicv_activated(vcpu->kvm) != kvm_vcpu_apicv_active(vcpu));
> > +	/* Check that CVE-2021-3656 can't happen again */
> > +	if (!svm_is_intercept(svm, INTERCEPT_VMSAVE) ||
> > +	    !svm_is_intercept(svm, INTERCEPT_VMSAVE))
> > +		WARN_ON(!(svm->vmcb->control.virt_ext &
> > +			  VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
> > +
> >   	sync_lapic_to_cr8(vcpu);
> >   	if (unlikely(svm->asid != svm->vmcb->control.asid)) {
> > 
> 
> While it's nice to be "proactive", this does adds some extra work. Maybe it
> should be under CONFIG_DEBUG_KERNEL.  It could be useful to make it into its
> own function so we can add similar intercept invariants in the same place.

I don't know that DEBUG_KERNEL will guard much, DEBUG_KERNEL=y is very common,
e.g. it's on by default in the x86 defconfigs.  I too agree it's nice to be
proactive, but this isn't that different than say failing to intercept CR3 loads
when shadow paging is enabled.

If we go down the path of effectively auditing KVM invariants, I'd rather we
commit fully and (a) add a dedicated Kconfig that is highly unlikely to be turned
on by accident and (b) audit a large number of invariants.