* [PATCH v2 0/2] KVM: x86: gmem populate fix and cleanups
@ 2026-06-30 21:37 Sean Christopherson
2026-06-30 21:37 ` [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE Sean Christopherson
2026-06-30 21:37 ` [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source Sean Christopherson
0 siblings, 2 replies; 11+ messages in thread
From: Sean Christopherson @ 2026-06-30 21:37 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Kiryl Shutsemau
Cc: Dave Hansen, Rick Edgecombe, kvm, x86, linux-coco, linux-kernel,
Sashiko Bot, Joerg Roedel, Yan Zhao, Ackerley Tng
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE
2026-06-30 21:37 [PATCH v2 0/2] KVM: x86: gmem populate fix and cleanups Sean Christopherson
@ 2026-06-30 21:37 ` Sean Christopherson
2026-07-01 21:15 ` Ackerley Tng
2026-06-30 21:37 ` [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source Sean Christopherson
1 sibling, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2026-06-30 21:37 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Kiryl Shutsemau
Cc: Dave Hansen, Rick Edgecombe, kvm, x86, linux-coco, linux-kernel,
Sashiko Bot, Joerg Roedel, Yan Zhao, Ackerley Tng
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
2026-06-30 21:37 [PATCH v2 0/2] KVM: x86: gmem populate fix and cleanups Sean Christopherson
2026-06-30 21:37 ` [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE Sean Christopherson
@ 2026-06-30 21:37 ` Sean Christopherson
2026-07-01 7:27 ` Yan Zhao
` (3 more replies)
1 sibling, 4 replies; 11+ messages in thread
From: Sean Christopherson @ 2026-06-30 21:37 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Kiryl Shutsemau
Cc: Dave Hansen, Rick Edgecombe, kvm, x86, linux-coco, linux-kernel,
Sashiko Bot, Joerg Roedel, Yan Zhao, Ackerley Tng
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(®ion, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
2026-06-30 21:37 ` [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source Sean Christopherson
@ 2026-07-01 7:27 ` Yan Zhao
2026-07-01 8:02 ` Binbin Wu
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Yan Zhao @ 2026-07-01 7:27 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Kiryl Shutsemau, Dave Hansen, Rick Edgecombe, kvm,
x86, linux-coco, linux-kernel, Sashiko Bot, Joerg Roedel,
Ackerley Tng
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(®ion, 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
2026-06-30 21:37 ` [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source Sean Christopherson
2026-07-01 7:27 ` Yan Zhao
@ 2026-07-01 8:02 ` Binbin Wu
2026-07-01 17:12 ` Sean Christopherson
2026-07-01 9:22 ` Kiryl Shutsemau
2026-07-02 2:32 ` Xiaoyao Li
3 siblings, 1 reply; 11+ messages in thread
From: Binbin Wu @ 2026-07-01 8:02 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Kiryl Shutsemau, Dave Hansen, Rick Edgecombe, kvm,
x86, linux-coco, linux-kernel, Sashiko Bot, Joerg Roedel,
Yan Zhao, Ackerley Tng
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
2026-06-30 21:37 ` [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source Sean Christopherson
2026-07-01 7:27 ` Yan Zhao
2026-07-01 8:02 ` Binbin Wu
@ 2026-07-01 9:22 ` Kiryl Shutsemau
2026-07-02 2:32 ` Xiaoyao Li
3 siblings, 0 replies; 11+ messages in thread
From: Kiryl Shutsemau @ 2026-07-01 9:22 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Dave Hansen, Rick Edgecombe, kvm, x86, linux-coco,
linux-kernel, Sashiko Bot, Joerg Roedel, Yan Zhao, Ackerley Tng
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
2026-07-01 8:02 ` Binbin Wu
@ 2026-07-01 17:12 ` Sean Christopherson
2026-07-02 1:12 ` Binbin Wu
0 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2026-07-01 17:12 UTC (permalink / raw)
To: Binbin Wu
Cc: Paolo Bonzini, Kiryl Shutsemau, Dave Hansen, Rick Edgecombe, kvm,
x86, linux-coco, linux-kernel, Sashiko Bot, Joerg Roedel,
Yan Zhao, Ackerley Tng
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?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE
2026-06-30 21:37 ` [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE Sean Christopherson
@ 2026-07-01 21:15 ` Ackerley Tng
2026-07-01 21:22 ` Sean Christopherson
0 siblings, 1 reply; 11+ messages in thread
From: Ackerley Tng @ 2026-07-01 21:15 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Kiryl Shutsemau
Cc: Dave Hansen, Rick Edgecombe, kvm, x86, linux-coco, linux-kernel,
Sashiko Bot, Joerg Roedel, Yan Zhao
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE
2026-07-01 21:15 ` Ackerley Tng
@ 2026-07-01 21:22 ` Sean Christopherson
0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2026-07-01 21:22 UTC (permalink / raw)
To: Ackerley Tng
Cc: Paolo Bonzini, Kiryl Shutsemau, Dave Hansen, Rick Edgecombe, kvm,
x86, linux-coco, linux-kernel, Sashiko Bot, Joerg Roedel,
Yan Zhao
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).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
2026-07-01 17:12 ` Sean Christopherson
@ 2026-07-02 1:12 ` Binbin Wu
0 siblings, 0 replies; 11+ messages in thread
From: Binbin Wu @ 2026-07-02 1:12 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Kiryl Shutsemau, Dave Hansen, Rick Edgecombe, kvm,
x86, linux-coco, linux-kernel, Sashiko Bot, Joerg Roedel,
Yan Zhao, Ackerley Tng
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
2026-06-30 21:37 ` [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source Sean Christopherson
` (2 preceding siblings ...)
2026-07-01 9:22 ` Kiryl Shutsemau
@ 2026-07-02 2:32 ` Xiaoyao Li
3 siblings, 0 replies; 11+ messages in thread
From: Xiaoyao Li @ 2026-07-02 2:32 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Kiryl Shutsemau
Cc: Dave Hansen, Rick Edgecombe, kvm, x86, linux-coco, linux-kernel,
Sashiko Bot, Joerg Roedel, Yan Zhao, Ackerley Tng
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.
>
> 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: Xiaoyao Li <xiaoyao.li@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(®ion, 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))
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-07-02 2:32 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-30 21:37 [PATCH v2 0/2] KVM: x86: gmem populate fix and cleanups Sean Christopherson
2026-06-30 21:37 ` [PATCH v2 1/2] KVM: SEV: Explicitly disallow NULL user address for SNP_LAUNCH_UPDATE Sean Christopherson
2026-07-01 21:15 ` Ackerley Tng
2026-07-01 21:22 ` Sean Christopherson
2026-06-30 21:37 ` [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source Sean Christopherson
2026-07-01 7:27 ` Yan Zhao
2026-07-01 8:02 ` Binbin Wu
2026-07-01 17:12 ` Sean Christopherson
2026-07-02 1:12 ` Binbin Wu
2026-07-01 9:22 ` Kiryl Shutsemau
2026-07-02 2:32 ` Xiaoyao Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox