Re: [PATCH 05/24] KVM: SEV: Use the size of the PSC header as the minimum size for PSC requests

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Insufficient bounds checking for Page State Change (PSC) requests allows out-of-bounds host memory reads and writes past the mapped 4KB GHCB page.
- [High] The GHCB mapping pinned via kvm_vcpu_map() on #VMGEXIT is permanently leaked if the vCPU is destroyed before the next VMRUN.
--

commit 2f8a93115345b3dcacdee9bdd3b3bd2f9154bfc6
Author: Sean Christopherson <seanjc@google.com>

KVM: SEV: Use the size of the PSC header as the minimum size for PSC requests

This commit updates the Page State Change VMGEXIT handler to use the size
of the PSC header as the minimum length for the scratch area. It stops
using exit_info_2 for the length, since the GHCB specification does not
provide length for PSC requests.

Cc: stable@vger.kernel.org

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 52703c9548568..cbb3040e07782 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -4559,7 +4559,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>  		vcpu->run->system_event.data[0] = control->ghcb_gpa;
>  		break;
>  	case SVM_VMGEXIT_PSC:
> -		ret = setup_vmgexit_scratch(svm, true, control->exit_info_2);
> +		ret = setup_vmgexit_scratch(svm, true, sizeof(struct psc_hdr));

[Severity: Critical]
This isn't a bug introduced by this patch, but by hardcoding the bounds check
to just the header size, does this allow out-of-bounds host memory reads and
writes past the mapped GHCB page?

If a malicious guest places the scratch area near the end of the GHCB shared
buffer, it can satisfy the bounds check for the 8-byte header in
setup_vmgexit_scratch():

setup_vmgexit_scratch() {
        ...
        scratch_gpa_end = scratch_gpa_beg + len;
        ...
        if (scratch_gpa_beg < ghcb_scratch_beg ||
            scratch_gpa_end > ghcb_scratch_end) {
            ...
}

However, later processing in snp_begin_psc() and __snp_complete_one_psc()
iterates up to VMGEXIT_PSC_MAX_COUNT, directly accessing and writing to
entries[idx]. Since the GHCB is a single page mapped via kvm_vcpu_map(),
accessing entries beyond the validated 8 bytes could result in accessing
host physical memory beyond the page boundary.

I see this is fixed later in the series by commit
89266ec65feabeba5d0f747da7115a19492fab4e and commit
4dd1fe3ae23c326b447d893657435197aa346458, which compute and check the correct
max length of the scratch area.

[Severity: High]
This is a pre-existing issue, but could the GHCB mapping pinned via
kvm_vcpu_map() earlier in sev_handle_vmgexit() be permanently leaked?

sev_handle_vmgexit() pins the GHCB page:

sev_handle_vmgexit() {
        ...
        if (kvm_vcpu_map(vcpu, ghcb_gpa >> PAGE_SHIFT, &svm->sev_es.ghcb_map)) {
        ...
}

Normally, this is unmapped on the next VMRUN in sev_es_unmap_ghcb(). However,
if the vCPU exits to userspace and the VM is subsequently destroyed without
running the vCPU again, the VCPU teardown path in sev_free_vcpu() does not
appear to unmap the GHCB. Would this result in a leaked pinned page and memory
mapping for every such destroyed vCPU?

I note this is addressed later in the series by commit
b2c42b3d82becdc801cc7f669caa3dbec0464c89.

>  		if (ret)
>  			break;

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