Re: [PATCH 4/4] kvm: sev: Acquire a writeable page reference for CPUID pages

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Incorrect unmap order for `kmap_local` mappings in the CPUID error reporting path leads to host kernel crashes.
- [High] Missing dirty tracking for user pages modified by the kernel, leading to potential silent data loss.
- [High] Data race and Use-After-Free on `kvm_memory_slot` between memslot teardown and `kvm_gmem_error_folio`.
--

commit 53ff0fbc410035f0e257e032a520eacbc8269a5c
Author: Joerg Roedel <joerg.roedel@amd.com>

kvm: sev: Acquire a writeable page reference for CPUID pages

When the PSP checks on a user-provided CPUID page fail KVM will write
back the detailed error information to the user-provided buffer.

Make sure this buffer is actually writable to not write the errors to
a read-only page.

Fixes: 2a62345b3052 ("KVM: guest_memfd: GUP source pages prior to populating guest memory")
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2415,6 +2415,7 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	struct kvm_memory_slot *memslot;
>  	long npages, count;
>  	void __user *src;
> +	int write;
>  
>  	if (!sev_snp_guest(kvm) || !sev->snp_context)
>  		return -EINVAL;

[ ... ]

> @@ -2475,7 +2476,10 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	sev_populate_args.sev_fd = argp->sev_fd;
>  	sev_populate_args.type = params.type;
>  
> -	count = kvm_gmem_populate(kvm, params.gfn_start, src, npages, 0,
> +	/* Acquire a write-reference for CPUID pages as kernel might write to it */
> +	write = params.type == KVM_SEV_SNP_PAGE_TYPE_CPUID;
> +
> +	count = kvm_gmem_populate(kvm, params.gfn_start, src, npages, write,
>  				  sev_gmem_post_populate, &sev_populate_args);

[Severity: High]
This isn't a bug introduced by this patch, but does kvm_gmem_populate()
handle dirtying the page correctly after sev_gmem_post_populate() modifies it?

When a user-provided CPUID page triggers SEV_RET_INVALID_PARAM, the error
path modifies the page directly via memcpy():

arch/x86/kvm/svm/sev.c:sev_gmem_post_populate() {
    ...
    memcpy(src_vaddr, dst_vaddr, PAGE_SIZE);
    ...
}

Since this bypasses hardware PTE dirty tracking, and kvm_gmem_populate()
releases the page without marking it dirty:

virt/kvm/guest_memfd.c:kvm_gmem_populate() {
    ...
    if (src_page)
        put_page(src_page);
    ...
}

Could this result in the userspace VMM permanently losing the error details if
the memory is file-backed or pushed to swap and discarded without writeback?


[Severity: Critical]
This is a pre-existing issue, but does sev_gmem_post_populate() unmap pages
in the correct order?

If __sev_issue_cmd() fails, it maps src_vaddr and dst_vaddr, but appears to
unmap them in FIFO order:

arch/x86/kvm/svm/sev.c:sev_gmem_post_populate() {
    ...
    kunmap_local(src_vaddr);
    kunmap_local(dst_vaddr);
    ...
}

Since the kmap_local API strictly requires LIFO unmapping, could an untrusted
VMM intentionally passing an invalid CPUID page trigger a host kernel crash or
corrupt the per-CPU kmap_ctrl stack?


[Severity: High]
This is a pre-existing issue, but is there a data race and use-after-free
risk on kvm_memory_slot between memslot teardown and kvm_gmem_error_folio()?

When KVM destroys a memslot, kvm_gmem_unbind() fast-paths the teardown if
there is no file:

virt/kvm/guest_memfd.c:kvm_gmem_unbind() {
    if (!file) {
        __kvm_gmem_unbind(slot, slot->gmem.file->private_data);
        return;
    }
    ...
}

This deliberately skips acquiring filemap_invalidate_lock(file->f_mapping).

If a hardware memory failure concurrently triggers kvm_gmem_error_folio(),
it iterates over f->bindings holding only filemap_invalidate_lock_shared().

Because xa_for_each_range() drops the internal RCU lock for each element,
could it yield the slot pointer right as kvm_gmem_unbind() completes and
frees it, leading to a dereference of freed memory here?

virt/kvm/guest_memfd.c:__kvm_gmem_invalidate_begin() {
    ...
    .start = slot->base_gfn + max(pgoff, start) - pgoff,
    ...
}

>  	if (count < 0) {
>  		argp->error = sev_populate_args.fw_error;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623091556.1500930-1-joro@8bytes.org?part=4