Hi, On Thu, 7 May 2026 at 21:22, Ackerley Tng via B4 Relay <[email protected]> wrote: > > From: Michael Roth <[email protected]> > > For vm_memory_attributes=1, in-place conversion/population is not > supported, so the initial contents necessarily must need to come > from a separate src address, which is enforced by the current > implementation. However, for vm_memory_attributes=0, it is possible for > guest memory to be initialized directly from userspace by mmap()'ing the > guest_memfd and writing to it while the corresponding GPA ranges are in > a 'shared' state before converting them to the 'private' state expected > by KVM_SEV_SNP_LAUNCH_UPDATE. > > Update the handling/documentation for KVM_SEV_SNP_LAUNCH_UPDATE to allow > for 'uaddr' to be set to NULL when vm_memory_attributes=0, which > SNP_LAUNCH_UPDATE will then use to determine when it should/shouldn't > copy in data from a separate memory location. Continue to enforce > non-NULL for the original vm_memory_attributes=1 case. > > Signed-off-by: Michael Roth <[email protected]> > [Added src_page check in error handling path when the firmware command fails] > [Dropped ifdef CONFIG_KVM_VM_MEMORY_ATTRIBUTES] > Signed-off-by: Ackerley Tng <[email protected]>
I'm not very familiar with the SEV-SNP populate flows, but it looks like Sashiko is on to something: https://sashiko.dev/#/patchset/20260507-gmem-inplace-conversion-v6-0-91ab5a8b19a4%40google.com?part=21 - a potential read-only page overwrite, because src_page is acquired via get_user_pages_fast() without the FOLL_WRITE flag, but is then overwritten via memcpy - an ordering violation with the kunmap_local() calls These predate this patch series and are just being touched by the 'src_page' addition, but if Sashiko's right, these should probably be fixed sooner rather than later. Cheers, /fuad > --- > Documentation/virt/kvm/x86/amd-memory-encryption.rst | 15 +++++++++++---- > arch/x86/kvm/svm/sev.c | 18 +++++++++++++----- > virt/kvm/kvm_main.c | 1 + > 3 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst > b/Documentation/virt/kvm/x86/amd-memory-encryption.rst > index b2395dd4769de..43085f65b2d85 100644 > --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst > +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst > @@ -503,7 +503,8 @@ secrets. > > It is required that the GPA ranges initialized by this command have had the > KVM_MEMORY_ATTRIBUTE_PRIVATE attribute set in advance. See the documentation > -for KVM_SET_MEMORY_ATTRIBUTES for more details on this aspect. > +for KVM_SET_MEMORY_ATTRIBUTES/KVM_SET_MEMORY_ATTRIBUTES2 for more details on > +this aspect. > > Upon success, this command is not guaranteed to have processed the entire > range requested. Instead, the ``gfn_start``, ``uaddr``, and ``len`` fields of > @@ -511,9 +512,15 @@ range requested. Instead, the ``gfn_start``, ``uaddr``, > and ``len`` fields of > remaining range that has yet to be processed. The caller should continue > calling this command until those fields indicate the entire range has been > processed, e.g. ``len`` is 0, ``gfn_start`` is equal to the last GFN in the > -range plus 1, and ``uaddr`` is the last byte of the userspace-provided source > -buffer address plus 1. In the case where ``type`` is > KVM_SEV_SNP_PAGE_TYPE_ZERO, > -``uaddr`` will be ignored completely. > +range plus 1, and ``uaddr`` (if specified) is the last byte of the > +userspace-provided source buffer address plus 1. > + > +In the case where ``type`` is KVM_SEV_SNP_PAGE_TYPE_ZERO, ``uaddr`` will be > +ignored completely. Otherwise, ``uaddr`` is required if > +kvm.vm_memory_attributes=1 and optional if kvm.vm_memory_attributes=0, since > +in the latter case guest memory can be initialized directly from userspace > +prior to converting it to private and passing the GPA range on to this > +interface. > > Parameters (in): struct kvm_sev_snp_launch_update > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index c2126b3c30724..bf10d24907a00 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2343,7 +2343,15 @@ 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)) > + /* > + * For vm_memory_attributes=1, in-place conversion/population is not > + * supported, so the initial contents necessarily need to come from a > + * separate src address. For vm_memory_attributes=0, this isn't > + * necessarily the case, since the pages may have been populated > + * directly from userspace before calling KVM_SEV_SNP_LAUNCH_UPDATE. > + */ > + if (vm_memory_attributes && > + sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && > !src_page) > return -EINVAL; > > ret = snp_lookup_rmpentry((u64)pfn, &assigned, &level); > @@ -2390,7 +2398,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, > gfn_t gfn, kvm_pfn_t pfn, > */ > 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) { > + sev_populate_args->fw_error == SEV_RET_INVALID_PARAM && src_page) > { > void *src_vaddr = kmap_local_page(src_page); > void *dst_vaddr = kmap_local_pfn(pfn); > > @@ -2422,8 +2430,8 @@ static int snp_launch_update(struct kvm *kvm, struct > kvm_sev_cmd *argp) > if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), > sizeof(params))) > return -EFAULT; > > - pr_debug("%s: GFN start 0x%llx length 0x%llx type %d flags %d\n", > __func__, > - params.gfn_start, params.len, params.type, params.flags); > + pr_debug("%s: GFN start 0x%llx length 0x%llx type %d flags %d src > %llx\n", __func__, > + params.gfn_start, params.len, params.type, params.flags, > params.uaddr); > > if (!params.len || !PAGE_ALIGNED(params.len) || params.flags || > (params.type != KVM_SEV_SNP_PAGE_TYPE_NORMAL && > @@ -2479,7 +2487,7 @@ static int snp_launch_update(struct kvm *kvm, struct > kvm_sev_cmd *argp) > > params.gfn_start += count; > params.len -= count * PAGE_SIZE; > - if (params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO) > + if (src && params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO) > params.uaddr += count * PAGE_SIZE; > > if (copy_to_user(u64_to_user_ptr(argp->data), ¶ms, > sizeof(params))) > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index ba195bb239aaa..3bf212fd99193 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -105,6 +105,7 @@ module_param(allow_unsafe_mappings, bool, 0444); > #ifdef CONFIG_KVM_VM_MEMORY_ATTRIBUTES > bool vm_memory_attributes = true; > module_param(vm_memory_attributes, bool, 0444); > +EXPORT_SYMBOL_FOR_KVM_INTERNAL(vm_memory_attributes); > #endif > DEFINE_STATIC_CALL_RET0(__kvm_get_memory_attributes, > kvm_get_memory_attributes_t); > EXPORT_SYMBOL_FOR_KVM_INTERNAL(STATIC_CALL_KEY(__kvm_get_memory_attributes)); > > -- > 2.54.0.563.g4f69b47b94-goog > >
