Re: [PATCH 4/4] kvm: svm: Support KVM_SEV_SNP_PAGE_TYPE_VMSA at SNP_LAUNCH_UPDATE

["Jörg Rödel" <joro@8bytes.org>]

On Thu, Jun 11, 2026 at 12:58:49PM +0000, sashiko-bot@kernel.org wrote:
> [Severity: Low]
> Is the sev_features variable necessary? It gets assigned below when
> processing a KVM_SEV_SNP_PAGE_TYPE_VMSA page but doesn't appear to be
> used anywhere else in this function.

Yes, that is a left-over and can be removed.

> [Severity: High]
> This is a pre-existing issue, but does this violate the LIFO unmapping
> requirement of the highmem API?
> 
> The kmap_local_page() documentation requires mappings to be unmapped in the
> exact reverse order. Here src_vaddr is mapped first, then dst_vaddr, but
> they are unmapped in the same order instead of reverse. Could this corrupt
> the internal stack on the next kmap/kunmap operations?

Right, I will fix it in this patch.

> [Severity: High]
> Will this new VMSA initialization path expose a use-after-free of KVM
> memslots during launch finish?
> 
> By setting sev->initial_vmsa_gpa to a valid page here, the ioctl path
> KVM_SEV_SNP_LAUNCH_FINISH can now call sev_es_vmsa_pa(), which looks up the
> slot:
> 
> arch/x86/kvm/svm/sev.c:sev_es_vmsa_pa() {
>     ...
>     slot = gfn_to_memslot(vcpu->kvm, gfn);
>     ...
> }
> 
> Since the launch finish ioctl takes kvm->lock and vcpu->mutex, but not
> srcu_read_lock(), could a concurrent memslot update swap and free the
> memslot before it is accessed in kvm_gmem_get_pfn()?

Will further investigate this. If there is an issue it is independent of this
series.

> [Severity: Critical]
> This isn't a bug introduced by this patch, but does the error recovery path
> for KVM_SEV_SNP_PAGE_TYPE_CPUID write to a potentially read-only page?
> 
> When handling the CPUID type, kvm_gmem_populate() calls get_user_pages_fast()
> with gup_flags = 0, requesting read-only access. If userspace mapped a
> read-only page like the global zero page, GUP will succeed.
> 
> If the firmware returns SEV_RET_INVALID_PARAM, sev_gmem_post_populate() maps
> the page with kmap_local_page() and copies the expected values into it:
> 
> memcpy(src_vaddr, dst_vaddr, PAGE_SIZE);
> 
> Could this illegally overwrite read-only memory and corrupt the global zero
> page?

Looks like this can happen, but not introduced by this series. It needs a
separate fix.

> [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?

This is correct, we will send a separate fix.

-Joerg