linux-kernel - Re: [PATCH] KVM: SVM: Fix SNP AP destroy race with VMRUN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z93zl54pdFJ2wtns@google.com>
Date: Fri, 21 Mar 2025 16:17:43 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org, 
	Paolo Bonzini <pbonzini@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Ingo Molnar <mingo@...hat.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Michael Roth <michael.roth@....com>
Subject: Re: [PATCH] KVM: SVM: Fix SNP AP destroy race with VMRUN

On Fri, Mar 21, 2025, Tom Lendacky wrote:
> On 3/18/25 08:47, Tom Lendacky wrote:
> > On 3/18/25 07:43, Tom Lendacky wrote:
> >>>> Very off-the-cuff, but I assume KVM_REQ_UPDATE_PROTECTED_GUEST_STATE just needs
> >>>> to be annotated with KVM_REQUEST_WAIT.
> >>>
> >>> Ok, nice. I wasn't sure if KVM_REQUEST_WAIT would be appropriate here.
> >>> This is much simpler. Let me test it out and resend if everything goes ok.
> >>
> >> So that doesn't work. I can still get an occasional #VMEXIT_INVALID. Let
> >> me try to track down what is happening with this approach...
> >
> > Looks like I need to use kvm_make_vcpus_request_mask() instead of just a
> > plain kvm_make_request() followed by a kvm_vcpu_kick().

Ugh, I was going to say "you don't need to do that", but I forgot that
kvm_vcpu_kick() subtly doesn't honor KVM_REQUEST_WAIT.

Ooof, I'm 99% certain that's causing bugs elsewhere.  E.g. arm64's KVM_REQ_SLEEP
uses the same "broken" pattern (LOL, which means that of course RISC-V does too).
In quotes, because kvm_vcpu_kick() is the one that sucks.

I would rather fix that a bit more directly and obviously.  IMO, converting to
smp_call_function_single() isntead of bastardizing smp_send_reschedule() is worth
doing regardless of the WAIT mess.  This will allow cleaning up a bunch of
make_request+kick pairs, it'll just take a bit of care to make sure we don't
create a WAIT where one isn't wanted (though those probably should have a big fat
comment anyways).

Compiled tested only.

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 5de20409bcd9..fd9d9a3ee075 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1505,7 +1505,16 @@ bool kvm_vcpu_block(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu);
 bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
-void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
+
+#ifndef CONFIG_S390
+void __kvm_vcpu_kick(struct kvm_vcpu *vcpu, bool wait);
+
+static inline void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
+{
+       __kvm_vcpu_kick(vcpu, false);
+}
+#endif
+
 int kvm_vcpu_yield_to(struct kvm_vcpu *target);
 void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu, bool yield_to_kernel_mode);
 
@@ -2253,6 +2262,14 @@ static __always_inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
        __kvm_make_request(req, vcpu);
 }
 
+#ifndef CONFIG_S390
+static inline void kvm_make_request_and_kick(int req, struct kvm_vcpu *vcpu)
+{
+       kvm_make_request(req, vcpu);
+       __kvm_vcpu_kick(vcpu, req & KVM_REQUEST_WAIT);
+}
+#endif
+
 static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
 {
        return READ_ONCE(vcpu->requests);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 201c14ff476f..2a5120e2e6b4 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3734,7 +3734,7 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);
 /*
  * Kick a sleeping VCPU, or a guest VCPU in guest mode, into host kernel mode.
  */
-void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
+void __kvm_vcpu_kick(struct kvm_vcpu *vcpu, bool wait)
 {
        int me, cpu;
 
@@ -3764,12 +3764,12 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
        if (kvm_arch_vcpu_should_kick(vcpu)) {
                cpu = READ_ONCE(vcpu->cpu);
                if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
-                       smp_send_reschedule(cpu);
+                       smp_call_function_single(cpu, ack_kick, NULL, wait);
        }
 out:
        put_cpu();
 }
-EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
+EXPORT_SYMBOL_GPL(__kvm_vcpu_kick);
 #endif /* !CONFIG_S390 */
 
 int kvm_vcpu_yield_to(struct kvm_vcpu *target)