[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <cc6f6e29ca844cccbefd9f2a3c0a25159ce3fecd.camel@redhat.com>
Date: Tue, 25 Oct 2022 16:37:27 +0300
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Paolo Bonzini <pbonzini@...hat.com>,
Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>,
x86@...nel.org, Thomas Gleixner <tglx@...utronix.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
"H. Peter Anvin" <hpa@...or.com>, stable@...r.kernel.org
Subject: Re: [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset
On Thu, 2022-10-20 at 15:33 +0000, Sean Christopherson wrote:
> On Thu, Oct 20, 2022, Maxim Levitsky wrote:
> > While not obivous, kvm_vcpu_reset leaves the nested mode by
>
> Please add () when referencing function, and wrap closer to ~75 chars.
>
> > clearing 'vcpu->arch.hflags' but it does so without all the
> > required housekeeping.
> >
> > This makes SVM and VMX continue to use vmcs02/vmcb02 while
>
> This bug should be impossible to hit on VMX as INIT and TRIPLE_FAULT unconditionally
> cause VM-Exit, i.e. will always be forwarded to L1.
True I guess as I found out as well, in VMX the physical CPU can't be reset while
in guest mode. I'll update the changelog.
>
> > the cpu is not in nested mode.
>
> Can you add a blurb to call out exactly how this bug can be triggered? Doesn't
> take much effort to suss out the "how", but it'd be nice to capture that info in
> the changelog.
I will add (in another patch) a selftest for this.
>
> > In particular, in SVM code, it makes the 'svm_free_nested'
> > free the vmcb02, while still in use, which later triggers
> > use after free and a kernel crash.
> >
> > This issue is assigned CVE-2022-3344
> >
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> > ---
> > arch/x86/kvm/x86.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index d86a8aae1471d3..313c4a6dc65e45 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -11931,6 +11931,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> > WARN_ON_ONCE(!init_event &&
> > (old_cr0 || kvm_read_cr3(vcpu) || kvm_read_cr4(vcpu)));
> >
> > + kvm_leave_nested(vcpu);
>
> Not a big deal, especially if/when nested_ops are turned into static_calls, but
> at the same time it's quite easy to do:
>
> if (is_guest_mode(vcpu))
> kvm_leave_nested(vcpu);
>
> I think it's worth adding a comment explaining how this can happen, and to also
> call out that EFER is cleared on INIT, i.e. that virtualization is disabled due
> to EFER.SVME=0. Unsurprisingly, I don't see anything in the APM that explicitly
> states what happens if INIT occurs in guest mode, i.e. it's not immediately obvious
> that forcing the vCPU back to L1 is architecturally correct.
>
>
> > kvm_lapic_reset(vcpu, init_event);
> >
> > vcpu->arch.hflags = 0;
>
> Maybe add a WARN above this to try and detect other potential issues? Kinda silly,
> but it'd at least help draw attention to the importance of hflags.
>
> E.g. this?
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4bd5f8a751de..c50fa0751a0b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11915,6 +11915,15 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> unsigned long old_cr0 = kvm_read_cr0(vcpu);
> unsigned long new_cr0;
>
> + /*
> + * SVM doesn't unconditionally VM-Exit on INIT and SHUTDOWN, thus it's
> + * possible to INIT the vCPU while L2 is active. Force the vCPU back
> + * into L1 as EFER.SVME is cleared on INIT (along with all other EFER
> + * bits), i.e. virtualization is disabled.
> + */
> + if (is_guest_mode(vcpu))
> + kvm_leave_nested(vcpu);
> +
> /*
> * Several of the "set" flows, e.g. ->set_cr0(), read other registers
> * to handle side effects. RESET emulation hits those flows and relies
> @@ -11927,6 +11936,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>
> kvm_lapic_reset(vcpu, init_event);
>
> + WARN_ON_ONCE(is_guest_mode(vcpu) || is_smm(vcpu));
> vcpu->arch.hflags = 0;
>
> vcpu->arch.smi_pending = 0;
>
Best regards,
Maxim Levitsky
Powered by blists - more mailing lists