Re: [PATCH 11/24] KVM: SEV: Make it more obvious when KVM is writing back the current PSC index

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A malicious guest can cause an infinite kernel loop and a soft lockup by concurrently modifying the GHCB scratch area during unsupported PSC operations.
--

commit 7f37f8948c706d884d14a7c69279c900524daced
Author: Sean Christopherson <seanjc@google.com>

KVM: SEV: Make it more obvious when KVM is writing back the current PSC index

This commit moves the guest-visible "cur_entry" index increment outside the
inner for-loop. It also adds a comment to clarify which code modifies trusted
versus guest-accessible data.

Message-ID: <20260501202250.2115252-12-seanjc@google.com>

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3947,7 +3947,7 @@ static int snp_begin_psc(struct vcpu_svm *svm)
>  	}
>  
>  	/* Find the start of the next range which needs processing. */
> -	for (idx = idx_start; idx <= idx_end; idx++, hdr->cur_entry++) {
> +	for (idx = idx_start; idx <= idx_end; idx++) {
>  		entry_start = READ_ONCE(entries[idx]);
>  
[ ... ]
> @@ -3974,6 +3974,14 @@ static int snp_begin_psc(struct vcpu_svm *svm)
>  
>  		if (npages)
>  			break;
> +
> +		/*
> +		 * Increment the guest-visible index to communicate the current
> +		 * entry back to the guest, e.g. in case of failure.  No need
> +		 * for READ_ONCE() as KVM doesn't consume the field, i.e. a
> +		 * misbehaving guest can only break itself.
> +		 */
> +		hdr->cur_entry++;
>  	}
>  
>  	if (idx > idx_end) {

[Severity: High]
This is a pre-existing issue, but since we're examining how a misbehaving 
guest affects the PSC processing loop, is it possible for a guest to trigger 
a soft lockup here?

Looking at snp_begin_psc(), if the guest submits an unsupported PSC operation,
we hit the default case and loop back:

next_range:
    idx_start = READ_ONCE(hdr->cur_entry);
    ...
    switch (entry_start.operation) {
    ...
    default:
        __snp_complete_one_psc(svm);
        goto next_range;
    }

If a malicious guest concurrently resets hdr->cur_entry back to 0 in the
GHCB shared memory, could the host get trapped continuously reading an
index of 0 and re-processing the same unsupported entry? 

Since there is no yield or exit to userspace in this default case, it seems 
this might trap the host thread in the kernel while holding vcpu->mutex,
leading to a persistent soft lockup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529183549.1104619-1-pbonzini@redhat.com?part=11