Re: [PATCH 4/4] kvm: svm: Support KVM_SEV_SNP_PAGE_TYPE_VMSA at SNP_LAUNCH_UPDATE

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing `srcu_read_lock()` when calling `gfn_to_memslot()` in `sev_es_vmsa_pa()`, leading to a potential Use-After-Free of KVM memslots.
- [Low] The local variable `sev_features` in `sev_gmem_post_populate()` is assigned but never used.

Pre-existing issues:
- [Critical] KVM writes firmware data to a potentially read-only userspace page during CPUID error recovery, which could corrupt the global zero page.
- [High] Out-of-order `kunmap_local()` calls violate the highmem API's strict LIFO unmapping requirement.
- [High] A malicious host userspace process can trigger a kernel warning (`WARN_ON_ONCE`) by passing a NULL user address (`uaddr = 0`) when issuing `KVM_SEV_SNP_LAUNCH_UPDATE` for any non-ZERO page type.
--

commit f2475c503a445642b6525297ce3c8dd1d2a1067f
Author: Joerg Roedel <joerg.roedel@amd.com>

kvm: svm: Support KVM_SEV_SNP_PAGE_TYPE_VMSA at SNP_LAUNCH_UPDATE

This commit adds support for setting a VMSA in guest physical memory
during the SEV-SNP launch process. It introduces a new page type,
KVM_SEV_SNP_PAGE_TYPE_VMSA, which is used for the BSP.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0c42af2e62016..52d0fa5ebdab1 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c

[ ... ]

> @@ -2532,7 +2546,9 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
>  	struct sev_gmem_populate_args *sev_populate_args = opaque;
>  	struct sev_data_snp_launch_update fw_args = {0};
>  	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> +	gpa_t gpa = gfn << PAGE_SHIFT;
>  	bool assigned = false;
> +	u64 sev_features = 0;
>  	int level;
>  	int ret;

[Severity: Low]
Is the sev_features variable necessary? It gets assigned below when
processing a KVM_SEV_SNP_PAGE_TYPE_VMSA page but doesn't appear to be
used anywhere else in this function.

[ ... ]

> @@ -2550,14 +2566,27 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
>  	if (src_page) {
>  		void *src_vaddr = kmap_local_page(src_page);
>  		void *dst_vaddr = kmap_local_pfn(pfn);
> +		struct sev_es_save_area *vmsa = dst_vaddr;
> +		bool accept_page = true;
>  
>  		memcpy(dst_vaddr, src_vaddr, PAGE_SIZE);
>  
> +		if (sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_VMSA) {
> +			accept_page = snp_check_launch_vmsa(sev, vmsa);
> +			if (accept_page)
> +				sev_features = vmsa->sev_features;
> +		}
>  
>  		kunmap_local(src_vaddr);
>  		kunmap_local(dst_vaddr);

[Severity: High]
This is a pre-existing issue, but does this violate the LIFO unmapping
requirement of the highmem API?

The kmap_local_page() documentation requires mappings to be unmapped in the
exact reverse order. Here src_vaddr is mapped first, then dst_vaddr, but
they are unmapped in the same order instead of reverse. Could this corrupt
the internal stack on the next kmap/kunmap operations?

[ ... ]

> @@ -2593,6 +2622,9 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
>  		kunmap_local(dst_vaddr);
>  	}
>  
> +	if (ret == 0 && sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_VMSA)
> +		sev->initial_vmsa_gpa = gpa;
> +

[Severity: High]
Will this new VMSA initialization path expose a use-after-free of KVM
memslots during launch finish?

By setting sev->initial_vmsa_gpa to a valid page here, the ioctl path
KVM_SEV_SNP_LAUNCH_FINISH can now call sev_es_vmsa_pa(), which looks up the
slot:

arch/x86/kvm/svm/sev.c:sev_es_vmsa_pa() {
    ...
    slot = gfn_to_memslot(vcpu->kvm, gfn);
    ...
}

Since the launch finish ioctl takes kvm->lock and vcpu->mutex, but not
srcu_read_lock(), could a concurrent memslot update swap and free the
memslot before it is accessed in kvm_gmem_get_pfn()?

>  out:
>  	if (ret)
>  		pr_debug("%s: error updating GFN %llx, return code %d (fw_error %d)\n",
> @@ -2620,12 +2652,22 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  
>  	if (!params.len || !PAGE_ALIGNED(params.len) || params.flags ||
>  	    (params.type != KVM_SEV_SNP_PAGE_TYPE_NORMAL &&
> +	     params.type != KVM_SEV_SNP_PAGE_TYPE_VMSA &&
>  	     params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO &&
>  	     params.type != KVM_SEV_SNP_PAGE_TYPE_UNMEASURED &&
>  	     params.type != KVM_SEV_SNP_PAGE_TYPE_SECRETS &&
>  	     params.type != KVM_SEV_SNP_PAGE_TYPE_CPUID))
>  		return -EINVAL;

[Severity: Critical]
This isn't a bug introduced by this patch, but does the error recovery path
for KVM_SEV_SNP_PAGE_TYPE_CPUID write to a potentially read-only page?

When handling the CPUID type, kvm_gmem_populate() calls get_user_pages_fast()
with gup_flags = 0, requesting read-only access. If userspace mapped a
read-only page like the global zero page, GUP will succeed.

If the firmware returns SEV_RET_INVALID_PARAM, sev_gmem_post_populate() maps
the page with kmap_local_page() and copies the expected values into it:

memcpy(src_vaddr, dst_vaddr, PAGE_SIZE);

Could this illegally overwrite read-only memory and corrupt the global zero
page?

> +	if (params.type == KVM_SEV_SNP_PAGE_TYPE_VMSA) {
> +		/* VMSA page are allowed only once */
> +		if (sev->initial_vmsa_gpa != INVALID_PAGE)
> +			return -EBUSY;
> +		/* Can only deploy a single page as VMSA */
> +		if (params.len != PAGE_SIZE)
> +			return -EINVAL;
> +	}
>  
>  	src = params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO ? NULL : u64_to_user_ptr(params.uaddr);

[Severity: High]
This is a pre-existing issue, but can a host userspace process trigger a
kernel warning by passing a NULL user address (uaddr = 0) here?

If params.uaddr is 0, src becomes NULL and passes the PAGE_ALIGNED(src)
check. kvm_gmem_populate() skips fetching the user page and passes
src_page = NULL to sev_gmem_post_populate().

That function then unconditionally evaluates:

WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO &&
             !src_page)

Since the type isn't ZERO, won't this allow an unprivileged user to spam
the kernel log?

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