Re: [PATCH 04/60] KVM: SVM: Inject NMIs when Restricted Injection is active

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] When an NMI injection is canceled while an interrupt is already pending in the doorbell page, KVM incorrectly merges both events into a single event_inj field and clears the doorbell, permanently dropping the interrupt.

Pre-existing issues:
- [High] WARN_ONCE in __sev_snp_inject can be triggered by a malicious guest setting an invalid ghcb_gpa, violating the API guideline against guest-reachable WARN_ONs.
- [High] Calling a sleepable mapping function (`kvm_vcpu_map`) while the task state is `TASK_INTERRUPTIBLE` breaks the wait loop in `kvm_vcpu_block()`, causing high CPU utilization (busy spin).
- [High] Data race on target vCPU's memslot cache (last_used_slot) via lockless kvm_vcpu_map() in sev_snp_blocked().
--

commit 457e56b8fb0a79fd5875e22fd1d0437c14618353
Author: Melody Wang <huibo.wang@amd.com>

KVM: SVM: Inject NMIs when Restricted Injection is active

This commit adds support for injecting NMIs into SEV-SNP guests when
Restricted Injection is active. It detects the active state and utilizes the
#HV doorbell communication from the GHCB specification to inject NMIs.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index cda83437382d7..44be2c9b8286d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -5408,7 +5408,10 @@ static void __sev_snp_inject(enum inject_type type, struct kvm_vcpu *vcpu)
>  		return;
>  	}
>  
> -	hvdb->events.vector = vcpu->arch.interrupt.nr;
> +	if (type == INJECT_NMI)

[Severity: High]
This isn't a bug introduced by this patch, but since this patch now calls
__sev_snp_inject() for NMIs, does this increase the exposure of the
guest-reachable WARN_ONCE() earlier in this function?

If a malicious guest modifies its memory map to remove the backing memslot for
the doorbell page, map_hvdb() will return NULL. Doesn't this allow an
unprivileged guest to intentionally trigger the WARN_ONCE(1) and potentially
cause a host denial of service if panic_on_warn is enabled?

> +		hvdb->events.nmi = 1;
> +	else
> +		hvdb->events.vector = vcpu->arch.interrupt.nr;
>  
>  	prepare_hv_injection(svm, hvdb);

[ ... ]

> @@ -5488,10 +5491,17 @@ void sev_snp_cancel_injection(struct kvm_vcpu *vcpu)
>  	/* Copy info back into event_inj field (replaces #HV) */
>  	svm->vmcb->control.event_inj = SVM_EVTINJ_VALID;
>  
> +	/*
> +	 * KVM only injects a single event each time (prepare_hv_injection),
> +	 * so when events.nmi is true, the vector will be zero
> +	 */
>  	if (hvdb->events.vector)
>  		svm->vmcb->control.event_inj |= hvdb->events.vector |
>  						SVM_EVTINJ_TYPE_INTR;
>  
> +	if (hvdb->events.nmi)
> +		svm->vmcb->control.event_inj |= SVM_EVTINJ_TYPE_NMI;
> +

[Severity: High]
Is it possible for both an NMI and an IRQ to be pending at the same time?

Looking at sev_snp_blocked(), it checks events.nmi and events.vector
independently based on the type. This means KVM could inject an NMI while an
unacknowledged IRQ is already in the doorbell page.

If a #VMEXIT happens and KVM calls sev_snp_cancel_injection(), wouldn't this
bitwise-OR both the IRQ vector and the NMI type into the single VMCB event_inj
field, overwriting the type to NMI but using the IRQ vector?

Since the code directly below wipes the doorbell page by setting
pending_events = 0, will the original interrupt be permanently lost?

>  	hvdb->events.pending_events = 0;
>  
>  out:

[ ... ]

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7253936c460c1..5255393986ccf 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c

[ ... ]

> @@ -3968,6 +3973,9 @@ bool svm_nmi_blocked(struct kvm_vcpu *vcpu)
>  	if (!gif_set(svm))
>  		return true;
>  
> +	if (sev_snp_is_rinj_active(vcpu))
> +		return sev_snp_blocked(INJECT_NMI, vcpu);
> +

[Severity: High]
This is a pre-existing issue, but adding sev_snp_blocked() to the NMI check
path expands an existing sleep-in-wait-loop regression.

During vCPU halting, kvm_vcpu_block() sets the task state to
TASK_INTERRUPTIBLE and waits in schedule(). If a wakeup check calls
kvm_vcpu_has_events() -> svm_nmi_blocked() -> sev_snp_blocked(), KVM maps
the doorbell page via kvm_vcpu_map().

Because kvm_vcpu_map() can take the mmap_read_lock and sleep, the scheduler
will reset the task state to TASK_RUNNING. Won't this cause the subsequent
schedule() call in kvm_vcpu_block() to return immediately, turning the halt
into a 100% CPU busy loop?

[Severity: High]
This isn't a bug introduced by this patch, but it expands a pre-existing data
race. Can this lockless doorbell page mapping cause a race on the target vCPU's
memslot cache?

When kvm_vcpu_on_spin() iterates over vCPUs to find a runnable target, it
calls kvm_vcpu_has_events() on foreign vCPUs. This eventually reaches
sev_snp_blocked() and calls kvm_vcpu_map() on the target vCPU.

Since kvm_vcpu_map() non-atomically modifies the target's
vcpu->last_used_slot cache, and this happens without holding the target's
vcpu->mutex, could this corrupt the memslot cache of the actively running
target vCPU?

>  	if (is_guest_mode(vcpu) && nested_exit_on_nmi(svm))
>  		return false;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=4