[PATCH v2 0/2] KVM: x86: gmem populate fix and cleanups

[PATCH v2 0/2] KVM: x86: gmem populate fix and cleanups

[Sean Christopherson]

Fix a user-triggerable WARN due to KVM not pre-checking that userspace
provided a source page for non-ZERO pages for SNP_LAUNCH_UPDATE, and then
clean up the equivalent TDX code to also explicitly check the incoming
source page *before* calling into guest_memfd, and to return -EINVAL, not
-EOPNOTSUPP.

v2:
 - Rewrite the SNP patch changelog.
 - Tweak the code to avoid checking KVM_SEV_SNP_PAGE_TYPE_ZERO twice.
 - Drop what is now effectively a sanity check in sev_gmem_post_populate(),
   so that we don't have to duplicate the logic when in-place conversion comes
   along.
 - Tack on the TDX change.

v1: https://lore.kernel.org/all/20260623091556.1500930-2-joro@8bytes.org

Joerg Roedel (1):
  KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE

Sean Christopherson (1):
  KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION
    source

 arch/x86/kvm/svm/sev.c | 11 +++++------
 arch/x86/kvm/vmx/tdx.c |  7 ++-----
 2 files changed, 7 insertions(+), 11 deletions(-)


base-commit: a204badd8432f93b7e862e7dac6db0fe3d65f370
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE

[Sean Christopherson]

From: Joerg Roedel <joerg.roedel@amd.com>

Explicitly reject a NULL userspace virtual address for the source page of
SNP_LAUNCH_UPDATE instead of relying on the post-populate callback to do
the check, and don't WARN on failure, as the scenario is blatantly user-
triggerable, as reported by Sashiko.  Waiting until post-populate to check
the address "works", but makes it unnecessarily difficult to see that KVM's
ABI is to disallow a NULL source page for non-ZERO pages.

Note, several existing VMMs pass a valid userspace address for the ZERO
case, i.e. KVM can't *require* the userspace address to be NULL for ZERO
pages, at least not without breaking userspace.

Fixes: dee5a47cc7a4 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command")
Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260611125849.9ED631F00893@smtp.kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 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;
 
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

> From: Joerg Roedel <joerg.roedel@amd.com>
>
> Explicitly reject a NULL userspace virtual address for the source page of
> SNP_LAUNCH_UPDATE instead of relying on the post-populate callback to do
> the check, and don't WARN on failure, as the scenario is blatantly user-
> triggerable, as reported by Sashiko.  Waiting until post-populate to check
> the address "works", but makes it unnecessarily difficult to see that KVM's
> ABI is to disallow a NULL source page for non-ZERO pages.
>
> Note, several existing VMMs pass a valid userspace address for the ZERO
> case, i.e. KVM can't *require* the userspace address to be NULL for ZERO
> pages, at least not without breaking userspace.
>
> Fixes: dee5a47cc7a4 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command")
> Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/all/20260611125849.9ED631F00893@smtp.kernel.org
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> Co-developed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  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);
>

I think separating validation from src assignment logic is better, like

    if (!PAGE_ALIGNED(params.uaddr))
	return -EINVAL;

    if (!params.uaddr && params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)
        return -EINVAL;

    src = params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO ? NULL :
u64_to_user_ptr(params.uaddr);

This version is also slightly stricter, since it enforces that
params.uaddr is aligned even if params.type is ZERO. (unless the
intention is to really not care about uaddr and permit an unaligned
uaddr?)

And then with conversions I'll add

    if (!gmem_in_place_conversion &&
        !params.uaddr && params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)


Just a suggestion:

Reviewed-by: Ackerley Tng <ackerleytng@google.com>

>  	npages = params.len / PAGE_SIZE;
>
> --
> 2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE

[Sean Christopherson]

On Wed, Jul 01, 2026, Ackerley Tng wrote:
> Sean Christopherson <seanjc@google.com> writes:
> > 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);
> >
> 
> I think separating validation from src assignment logic is better, like
> 
>     if (!PAGE_ALIGNED(params.uaddr))
> 	return -EINVAL;
> 
>     if (!params.uaddr && params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)
>         return -EINVAL;
> 
>     src = params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO ? NULL :
> u64_to_user_ptr(params.uaddr);
>
> This version is also slightly stricter, since it enforces that
> params.uaddr is aligned even if params.type is ZERO. 

That's technically an ABI change since KVM fully ignored params.uaddr before
this, i.e. is a potentially breaking change.  As noted in the changelog, KVM
should have required the address to be '0', but unfortunately we can't do that
without breaking userspace.

I highly doubt requiring the address to be page-aligned would break userspace,
but KVM also gains nothing from such a requirement, and from an ABI perspective,
placing restrictions on a value that is completely ignored is rather bizarre.

> (unless the intention is to really not care about uaddr and permit an
> unaligned uaddr?)

I don't think there was any intention (which is the whole problem).

[PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[Sean Christopherson]

Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
consistent between TDX and SNP (for LAUNCH_UPDATE).  EOPNOTSUPP was chosen
to be a forward-looking error code for when guest_memfd supports in-place
conversion, but even when in-place conversion comes along, it's an awkward
error code as KVM is deliberately choosing to disallow virtual address '0',
which is technically a legal userspace address.  I.e. it's not so much a
lack of support as it is that KVM reserves address '0' to simplify KVM's
internal implementation.

Opportunistically move the check so that it's co-located with the other
checks on the userspace address, and so that it's more obvious that a NULL
source address is explicitly disallowed.

Fixes: 2a62345b3052 ("KVM: guest_memfd: GUP source pages prior to populating guest memory")
Cc: Yan Zhao <yan.y.zhao@intel.com>
Cc: Ackerley Tng <ackerleytng@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 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))
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[sashiko-bot]

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

Pre-existing issues:
- [High] The `region.nr_pages << PAGE_SHIFT` calculation can overflow a 64-bit integer, bypassing GPA boundary validation and leading to a host denial of service.
--

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

KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
consistent between TDX and SNP.

> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index ffe9d0db58c59..b0ec054732b90 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c

[ ... ]

> @@ -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))

[Severity: High]
This is a pre-existing issue, but in tdx_vcpu_init_mem_region(), does the
calculation region.nr_pages << PAGE_SHIFT overflow a 64-bit integer?

If userspace calls KVM_TDX_INIT_MEM_REGION with region.nr_pages set to a
value where the top 12 bits are non-zero (e.g., (1ULL << 52) + 1), the left
shift by PAGE_SHIFT discards the top 12 bits, evaluating to a small value.

Does this allow a malicious host userspace to bypass the overflow check
region.gpa + (region.nr_pages << PAGE_SHIFT) <= region.gpa and the
vt_is_tdx_private_gpa bounds check?

If bypassed, the while loop would then iterate based on the large un-shifted
region.nr_pages value. If a guest_memfd is mapped at a shared GPA and
initialization is started just before it, the loop could incrementally walk
into the shared GPA space.

Since KVM invokes tdh_mr_extend on that shared GPA, which the TDX module
rejects, could this trigger KVM's TDX_BUG_ON_2 and cause a host denial of
service?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630213711.479692-1-seanjc@google.com?part=2

Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[Ackerley Tng]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The `region.nr_pages << PAGE_SHIFT` calculation can overflow a 64-bit integer, bypassing GPA boundary validation and leading to a host denial of service.
> --
>
> commit a54da29c3e4be3226e4ebf9df2f4182c350df998
> Author: Sean Christopherson <seanjc@google.com>
>
> KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
>
> Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
> pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
> consistent between TDX and SNP.
>
>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>> index ffe9d0db58c59..b0ec054732b90 100644
>> --- a/arch/x86/kvm/vmx/tdx.c
>> +++ b/arch/x86/kvm/vmx/tdx.c
>
> [ ... ]
>
>> @@ -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 ||

Since we're changing this, how about separating/grouping conditions so
that adding validation is clearly different from alignment changes?
Perhaps like

    if (!region.source_addr)
	return -EINVAL;

    if (!PAGE_ALIGNED(region.source_addr) || !PAGE_ALIGNED(region.gpa))
	return -EINVAL;

And this one might need more code if we're fixing the issue Sashiko
pointed out.

    if (overflow)
	return -EINVAL;

>> +	    !PAGE_ALIGNED(region.gpa) || !region.nr_pages ||

Looking at this again, why should region.nr_pages == 0 be -EINVAL? Why
not do an early exit and return 0?

>>  	    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))
>
> [Severity: High]
> This is a pre-existing issue, but in tdx_vcpu_init_mem_region(), does the
> calculation region.nr_pages << PAGE_SHIFT overflow a 64-bit integer?
>

From what I can tell I think this won't cause much damage outside of
this guest, and since INIT_MEM_REGION is called at the start, this
overflow is probably just busywork by the potential attacker, but I
think it's still worth adding an overflow check in case more code gets
added that could actually cause issues in future.

> If userspace calls KVM_TDX_INIT_MEM_REGION with region.nr_pages set to a
> value where the top 12 bits are non-zero (e.g., (1ULL << 52) + 1), the left
> shift by PAGE_SHIFT discards the top 12 bits, evaluating to a small value.
>
> Does this allow a malicious host userspace to bypass the overflow check
> region.gpa + (region.nr_pages << PAGE_SHIFT) <= region.gpa and the
> vt_is_tdx_private_gpa bounds check?
>
> If bypassed, the while loop would then iterate based on the large un-shifted
> region.nr_pages value. If a guest_memfd is mapped at a shared GPA and
> initialization is started just before it, the loop could incrementally walk
> into the shared GPA space.
>
> Since KVM invokes tdh_mr_extend on that shared GPA, which the TDX module
> rejects, could this trigger KVM's TDX_BUG_ON_2 and cause a host denial of
> service?

TDX_BUG_ON_2 is not a BUG_ON and won't take down the host.

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260630213711.479692-1-seanjc@google.com?part=2

Not a huge deal if we just go ahead with this change, thanks for moving
this check up front ahead of calling kvm_gmem_populate()!

Reviewed-by: Ackerley Tng <ackerleytng@google.com>

Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[Sean Christopherson]

On Wed, Jul 01, 2026, Ackerley Tng wrote:
> sashiko-bot@kernel.org writes:
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [High] The `region.nr_pages << PAGE_SHIFT` calculation can overflow a 64-bit integer, bypassing GPA boundary validation and leading to a host denial of service.
> > --
> >
> > commit a54da29c3e4be3226e4ebf9df2f4182c350df998
> > Author: Sean Christopherson <seanjc@google.com>
> >
> > KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
> >
> > Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
> > pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
> > consistent between TDX and SNP.
> >
> >> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> >> index ffe9d0db58c59..b0ec054732b90 100644
> >> --- a/arch/x86/kvm/vmx/tdx.c
> >> +++ b/arch/x86/kvm/vmx/tdx.c
> >
> > [ ... ]
> >
> >> @@ -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 ||
> 
> Since we're changing this, how about separating/grouping conditions so
> that adding validation is clearly different from alignment changes?
> Perhaps like
> 
>     if (!region.source_addr)
> 	return -EINVAL;
> 
>     if (!PAGE_ALIGNED(region.source_addr) || !PAGE_ALIGNED(region.gpa))
> 	return -EINVAL;

I'd rather not do that as part of this patch.  I'm not opposed to refactoring
this code, but I don't want to make the diff harder to read by throwing in
non-trivial changes.

> And this one might need more code if we're fixing the issue Sashiko
> pointed out.
> 
>     if (overflow)
> 	return -EINVAL;
> 
> >> +	    !PAGE_ALIGNED(region.gpa) || !region.nr_pages ||
> 
> Looking at this again, why should region.nr_pages == 0 be -EINVAL? Why
> not do an early exit and return 0?

Because nr_pages == 0 is nonsensical.  And as a general rule, don't return success
for uAPI unless it's necessary, because it's much, much harder to extend
functionality if a path can succeed than if a path is guaranteed to fail.

E.g. very hypothetically, if we want to repurpose '0' to mean "delete this page"
or something, then returning -EINVAL now provides a more viable route to supporting
such an operation.

Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[Yan Zhao]

On Tue, Jun 30, 2026 at 02:37:11PM -0700, Sean Christopherson wrote:
> Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
> pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
> consistent between TDX and SNP (for LAUNCH_UPDATE).  EOPNOTSUPP was chosen
> to be a forward-looking error code for when guest_memfd supports in-place
> conversion, but even when in-place conversion comes along, it's an awkward
> error code as KVM is deliberately choosing to disallow virtual address '0',
> which is technically a legal userspace address.  I.e. it's not so much a
> lack of support as it is that KVM reserves address '0' to simplify KVM's
> internal implementation.
> 
> Opportunistically move the check so that it's co-located with the other
> checks on the userspace address, and so that it's more obvious that a NULL
> source address is explicitly disallowed.
> 
> Fixes: 2a62345b3052 ("KVM: guest_memfd: GUP source pages prior to populating guest memory")
> Cc: Yan Zhao <yan.y.zhao@intel.com>
> Cc: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Yan Zhao <yan.y.zhao@intel.com>
Tested-by: Yan Zhao <yan.y.zhao@intel.com>

> ---
>  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))
> -- 
> 2.55.0.rc0.799.gd6f94ed593-goog
> 

Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[Binbin Wu]

On 7/1/2026 5:37 AM, Sean Christopherson wrote:
> Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
> pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
> consistent between TDX and SNP (for LAUNCH_UPDATE).  EOPNOTSUPP was chosen
> to be a forward-looking error code for when guest_memfd supports in-place
> conversion, but even when in-place conversion comes along, it's an awkward
> error code as KVM is deliberately choosing to disallow virtual address '0',
> which is technically a legal userspace address.  I.e. it's not so much a
> lack of support as it is that KVM reserves address '0' to simplify KVM's
> internal implementation.

Nit:
Do you think it's worth calling this out in the documentation?

> 
> Opportunistically move the check so that it's co-located with the other
> checks on the userspace address, and so that it's more obvious that a NULL
> source address is explicitly disallowed.
> 
> Fixes: 2a62345b3052 ("KVM: guest_memfd: GUP source pages prior to populating guest memory")
> Cc: Yan Zhao <yan.y.zhao@intel.com>
> Cc: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Binbin Wu <binbin.wu@linxu.intel.com>

Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[Sean Christopherson]

On Wed, Jul 01, 2026, Binbin Wu wrote:
> On 7/1/2026 5:37 AM, Sean Christopherson wrote:
> > Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
> > pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
> > consistent between TDX and SNP (for LAUNCH_UPDATE).  EOPNOTSUPP was chosen
> > to be a forward-looking error code for when guest_memfd supports in-place
> > conversion, but even when in-place conversion comes along, it's an awkward
> > error code as KVM is deliberately choosing to disallow virtual address '0',
> > which is technically a legal userspace address.  I.e. it's not so much a
> > lack of support as it is that KVM reserves address '0' to simplify KVM's
> > internal implementation.
> 
> Nit:
> Do you think it's worth calling this out in the documentation?

Yes, though that can be done separate since this series doesn't change ABI.
E.g. we can probably do it opportunistically as part of the in-place conversion
series?

Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[Binbin Wu]

On 7/2/2026 1:12 AM, Sean Christopherson wrote:
> On Wed, Jul 01, 2026, Binbin Wu wrote:
>> On 7/1/2026 5:37 AM, Sean Christopherson wrote:
>>> Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
>>> pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
>>> consistent between TDX and SNP (for LAUNCH_UPDATE).  EOPNOTSUPP was chosen
>>> to be a forward-looking error code for when guest_memfd supports in-place
>>> conversion, but even when in-place conversion comes along, it's an awkward
>>> error code as KVM is deliberately choosing to disallow virtual address '0',
>>> which is technically a legal userspace address.  I.e. it's not so much a
>>> lack of support as it is that KVM reserves address '0' to simplify KVM's
>>> internal implementation.
>>
>> Nit:
>> Do you think it's worth calling this out in the documentation?
> 
> Yes, though that can be done separate since this series doesn't change ABI.
> E.g. we can probably do it opportunistically as part of the in-place conversion
> series?

Yes, make sense.

Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[Kiryl Shutsemau]

On Tue, Jun 30, 2026 at 02:37:11PM -0700, Sean Christopherson wrote:
> Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
> pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
> consistent between TDX and SNP (for LAUNCH_UPDATE).  EOPNOTSUPP was chosen
> to be a forward-looking error code for when guest_memfd supports in-place
> conversion, but even when in-place conversion comes along, it's an awkward
> error code as KVM is deliberately choosing to disallow virtual address '0',
> which is technically a legal userspace address.  I.e. it's not so much a
> lack of support as it is that KVM reserves address '0' to simplify KVM's
> internal implementation.
> 
> Opportunistically move the check so that it's co-located with the other
> checks on the userspace address, and so that it's more obvious that a NULL
> source address is explicitly disallowed.
> 
> Fixes: 2a62345b3052 ("KVM: guest_memfd: GUP source pages prior to populating guest memory")
> Cc: Yan Zhao <yan.y.zhao@intel.com>
> Cc: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Acked-by: Kiryl Shutsemau (Meta) <kas@kernel.org>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov