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

[Sean Christopherson <seanjc@google.com>]

Please capitalize the scope, i.e. "KVM: SEV:".

On Tue, Jun 23, 2026, Jörg Rödel wrote:
> From: Joerg Roedel <joerg.roedel@amd.com>
> 
> Sashiko reported on an unrelated patch:
> 
>   [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?

Use Reported-by: + Closes to capture Sashiko's effecitve bug report instead of
copy+pasting the finding.  There's no reason to treat Sashiko any differently
than any other bot.

> The assessment is correct, so check for this condition earlier in the
> snp_launch_update() path to avoid the WARN_ON_ONCE.
> 
> Fixes: dee5a47cc7a45 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command")
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  arch/x86/kvm/svm/sev.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 6c6a6d663e29..41dcba5180ca 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.

Meh, that's pretty obvious from the code.

> +	 */
> +	if (src == NULL && params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)

I think I'd prefer this over checking for KVM_SEV_SNP_PAGE_TYPE_ZERO twice,
especially since the PAGE_ALIGNED() check for the NULL pointer case is rather
weird.

	if (params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO)
		src = NULL;
	else if (!params.uaddr || !PAGE_ALIGNED(params.uaddr))
		return -EINVAL;
	else
		src = u64_to_user_ptr(params.uaddr);


> +		return -EINVAL;

Gah, we created quite the mess for ourselves.  TDX returns -EOPNOTSUPP instead
of -EINVAL, I guess as a placeholder for in-place conversion?  I don't care which
error code is returned, but I do want KVM to be consistent.

We should also adjust TDX to pre-check the source address, because checking only
in the post-populate flow subtly relies on tdx_vcpu_init_mem_region() returning
immediately on error.  If that weren't the case (ignoring for the moment that
continuing on would be nonsensical), KVM would advace the address by PAGE_SIZE
and suddenly a NULL userspace address becomes non-NULL.

I also think it makes sense to drop the WARN in sev_gmem_post_populate(), it's
completely redundant once this bug is fixed.

Ugh, and both SNP and TDX fail to account for tags, and fail to check for
striding into kernel space.  Which I suppose is fine, since gup() handles those
correctly.  And I don't see a strong argument for disallowing tagged addresses,
because unlike the userspace address for memslots, KVM doesn't keep the address
around long-term.

So over two patches, the below?  I can send a v2, I've already got changelogs
written (I was fiddling around with extracting and reusing kvm_set_memory_region()'s
checks on the userspace address+size, but as above, convinced myself that KVM
should continue to allow tagged addresses for SNP and TDX).

---
 arch/x86/kvm/svm/sev.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 74fb15551e83..621a2eaa58f2 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2330,9 +2330,6 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
 	int level;
 	int ret;
 
-	if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src_page))
-		return -EINVAL;
-
 	ret = snp_lookup_rmpentry((u64)pfn, &assigned, &level);
 	if (ret || assigned) {
 		pr_debug("%s: Failed to ensure GFN 0x%llx RMP entry is initial shared state, ret: %d assigned: %d\n",
@@ -2421,10 +2418,12 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	     params.type != KVM_SEV_SNP_PAGE_TYPE_CPUID))
 		return -EINVAL;
 
-	src = params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO ? NULL : u64_to_user_ptr(params.uaddr);
-
-	if (!PAGE_ALIGNED(src))
+	if (params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO)
+		src = NULL;
+	else if (!params.uaddr || !PAGE_ALIGNED(params.uaddr))
 		return -EINVAL;
+	else
+		src = u64_to_user_ptr(params.uaddr);
 
 	npages = params.len / PAGE_SIZE;

---
 arch/x86/kvm/vmx/tdx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index ffe9d0db58c5..b0ec054732b9 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -3198,9 +3198,6 @@ static int tdx_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
 	if (KVM_BUG_ON(kvm_tdx->page_add_src, kvm))
 		return -EIO;
 
-	if (!src_page)
-		return -EOPNOTSUPP;
-
 	kvm_tdx->page_add_src = src_page;
 	ret = kvm_tdp_mmu_map_private_pfn(arg->vcpu, gfn, pfn);
 	kvm_tdx->page_add_src = NULL;
@@ -3247,8 +3244,8 @@ static int tdx_vcpu_init_mem_region(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *c
 	if (copy_from_user(&region, u64_to_user_ptr(cmd->data), sizeof(region)))
 		return -EFAULT;
 
-	if (!PAGE_ALIGNED(region.source_addr) || !PAGE_ALIGNED(region.gpa) ||
-	    !region.nr_pages ||
+	if (!PAGE_ALIGNED(region.source_addr) || !region.source_addr ||
+	    !PAGE_ALIGNED(region.gpa) || !region.nr_pages ||
 	    region.gpa + (region.nr_pages << PAGE_SHIFT) <= region.gpa ||
 	    !vt_is_tdx_private_gpa(kvm, region.gpa) ||
 	    !vt_is_tdx_private_gpa(kvm, region.gpa + (region.nr_pages << PAGE_SHIFT) - 1))
--