Re: [PATCH 1/4] kvm: sev: Fix user-space triggerable WARN_ON on snp_launch_update path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `snp_launch_update` unconditionally returns `-EIO` when `kvm_gmem_populate` fails, discarding critical error codes like `-EINTR`, `-EFAULT`, and `-ENOMEM`.
- [High] `sev_gmem_post_populate` unmaps `kmap_local` pointers in the wrong order in the CPUID firmware error handling block.
--

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

kvm: sev: Fix user-space triggerable WARN_ON on snp_launch_update path

This commit avoids a WARN_ON_ONCE in the snp_launch_update() path by explicitly
checking if a user-space process passed a NULL source address when the page
type is not KVM_SEV_SNP_PAGE_TYPE_ZERO.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 6c6a6d663e296..41dcba5180cad 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2438,6 +2438,13 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	if (!PAGE_ALIGNED(src))
>  		return -EINVAL;
>  
> +	/*
> +	 * Make sure user-mode did not pass NULL as src with
> +	 * type != KVM_SEV_SNP_PAGE_TYPE_ZERO.
> +	 */
> +	if (src == NULL && params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)
> +		return -EINVAL;
> +
>  	npages = params.len / PAGE_SIZE;

[Severity: High]
This is a pre-existing issue, but does snp_launch_update() unconditionally
return -EIO when kvm_gmem_populate() fails?

arch/x86/kvm/svm/sev.c:snp_launch_update() {
...
	count = kvm_gmem_populate(kvm, params.gfn_start, src, npages, write,
				  sev_gmem_post_populate, &sev_populate_args);
	if (count < 0) {
		argp->error = sev_populate_args.fw_error;
		pr_debug("%s: kvm_gmem_populate failed, ret %ld (fw_error %d)\n",
			 __func__, count, argp->error);
		return -EIO;
	}
...
}

Returning -EIO unconditionally here discards important error codes like
-EINTR, -EFAULT, and -ENOMEM from kvm_gmem_populate().

A userspace process could receive a signal (e.g. SIGALRM) during the ioctl
execution, causing kvm_gmem_populate() to return -EINTR, which gets
overwritten with -EIO.

This breaks standard Linux signal restart semantics and could cause VM
creation to fail under signal pressure.

[Severity: High]
This is also a pre-existing issue, but are the kmap_local pointers unmapped
in the wrong order in sev_gmem_post_populate()?

arch/x86/kvm/svm/sev.c:sev_gmem_post_populate() {
...
	if (ret && !snp_page_reclaim(kvm, pfn) &&
	    sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_CPUID &&
	    sev_populate_args->fw_error == SEV_RET_INVALID_PARAM) {
		void *src_vaddr = kmap_local_page(src_page);
		void *dst_vaddr = kmap_local_pfn(pfn);

		memcpy(src_vaddr, dst_vaddr, PAGE_SIZE);

		kunmap_local(src_vaddr);
		kunmap_local(dst_vaddr);
	}
...
}

kunmap_local() requires pointers to be unmapped in strictly reverse LIFO
order.

Here, src_vaddr is mapped first, then dst_vaddr, but src_vaddr is also
unmapped first.

This violates the API requirement and can trigger kernel panics via BUG_ON()
when CONFIG_DEBUG_KMAP_LOCAL is enabled.

While an earlier patch in this series ('kvm: sev: Unmap pages in correct
order in sev_gmem_post_populate()') fixed this in the first error block of
the function, it looks like this second error block was missed.

This can be triggered by a userspace VMM passing invalid CPUID data to
KVM_SEV_SNP_LAUNCH_UPDATE, causing firmware to reject the page.

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