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] [day] [month] [year] [list]

Message-ID: <Z-Q2uQ0perBQiZh-@google.com>
Date: Wed, 26 Mar 2025 10:17:45 -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 Wed, Mar 26, 2025, Tom Lendacky wrote:
> On 3/25/25 12:49, Tom Lendacky wrote:
> > On 3/21/25 18:17, Sean Christopherson wrote:
> >> 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).

...

> >> @@ -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);
> > 
> > In general, this approach works. However, this change triggered
> > 
> >  WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
> > 	      && !oops_in_progress);
> > 
> > in kernel/smp.c.

Drat, I forgot that smp_call_function_xxx() can deadlock even if wait=false due
to needing to take locks to set the callback function.

> Is keeping the old behavior desirable when IRQs are disabled? Something
> like:
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a6fedcadd036..81cbc55eac3a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3754,8 +3754,14 @@ void __kvm_vcpu_kick(struct kvm_vcpu *vcpu, bool wait)
>  	 */
>  	if (kvm_arch_vcpu_should_kick(vcpu)) {
>  		cpu = READ_ONCE(vcpu->cpu);
> -		if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
> -			smp_call_function_single(cpu, ack_kick, NULL, wait);
> +		if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) {
> +			WARN_ON_ONCE(wait && irqs_disabled());
> +
> +			if (irqs_disabled())
> +				smp_send_reschedule(cpu);
> +			else
> +				smp_call_function_single(cpu, ack_kick, NULL, wait);
> +		}
>  	}
>  out:
>  	put_cpu();

That, or keying off wait, and letting smp_call_function_xxx() yell about trying
to use it with IRQs disabled, i.e.

			if (wait)
				smp_call_function_single(cpu, ack_kick, NULL, wait);
			else
				smp_send_reschedule(cpu);

My vote would be for the checking "wait", so that the behavior is consistent for
a given request.