Re: [PATCH v3 6/8] KVM: SEV: Add SNP vCPU state get and set commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] KVM: SEV: misuse of INVALID_PAGE for ghcb_gpa in snp_set/get_vcpu_state()
- [High] KVM: SEV: premature gmem fault-in and RMP update in snp_set_vcpu_state()
- [High] KVM: SEV: missing guest_state_protected check in snp_set_vcpu_state()
- [High] KVM: SEV: setting vmsa_pa in snp_set_vcpu_state() bypasses pre_sev_run()
- [Medium] KVM: SEV: unlocked sev->snp_context read in snp_get/set_vcpu_state()
- [Medium] KVM: SEV: stale VMSA state in snp_get_vcpu_state() on pending reload request
- [Medium] KVM: SEV: snp_get_vcpu_state() hides VMSA GPA when VMSA reload is pending
--

commit 58d63c955c7c0bdc5747b6eafd39cb3beae4ba9d
Author: Joerg Roedel <joerg.roedel@amd.com>

KVM: SEV: Add SNP vCPU state get and set commands

This commit adds KVM_SEV_SNP_GET_VCPU_STATE and KVM_SEV_SNP_SET_VCPU_STATE
as vCPU-scoped KVM_MEMORY_ENCRYPT_OP commands and advertises
KVM_CAP_SNP_VCPU_STATE so userspace can configure and query guest-owned VMSA
and GHCB GPAs per vCPU.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 974651a70a7dd..ff696e21fe41b 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c

[ ... ]

> @@ -2606,6 +2608,89 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	return ret;
>  }
>  
> +static int snp_get_vcpu_state(struct kvm_vcpu *vcpu,
> +			      struct kvm_sev_cmd *argp)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct kvm *kvm = vcpu->kvm;
> +	struct kvm_sev_snp_vcpu_state state = {};
> +
> +	if (!is_sev_snp_guest(vcpu))
> +		return -ENOTTY;
> +	if (!to_kvm_sev_info(kvm)->snp_context)
> +		return -EINVAL;

[Severity: Medium]
Is it safe for snp_get_vcpu_state() and snp_set_vcpu_state() to read
sev->snp_context under vcpu->mutex without holding kvm->lock?

In snp_launch_start(), sev->snp_context is assigned under kvm->lock before
SEV_CMD_SNP_LAUNCH_START and snp_bind_asid() execute, and cleared via
snp_decommission_context() if either step fails:

arch/x86/kvm/svm/sev.c:snp_launch_start() {
	...
	sev->snp_context = snp_context_create(kvm, argp);
	if (!sev->snp_context)
		return -ENOTTY;
	...
e_free_context:
	snp_decommission_context(kvm);
	return rc;
}

If userspace invokes KVM_SEV_SNP_GET_VCPU_STATE or
KVM_SEV_SNP_SET_VCPU_STATE concurrently with KVM_SEV_SNP_LAUNCH_START, can
snp_set_vcpu_state() observe a transiently non-NULL sev->snp_context and
install state (including calling kvm_gmem_get_pfn() ->
sev_gmem_make_private()) before snp_bind_asid() has bound the asid or while
snp_launch_start() is failing and decommissioning the context?

> +
> +	guard(mutex)(&svm->sev_es.snp_vmsa_mutex);
> +
> +	if (VALID_PAGE(svm->sev_es.snp_guest_vmsa_gpa) &&
> +	    VALID_PAGE(svm->vmcb->control.vmsa_pa)) {
> +		state.vmsa_gpa = svm->sev_es.snp_guest_vmsa_gpa;
> +		state.valid_fields |= KVM_SEV_SNP_VCPU_STATE_VMSA_VALID;
> +	}

[Severity: Medium]
As reported by Sashiko on v2 [1], does checking
VALID_PAGE(svm->vmcb->control.vmsa_pa) here cause snp_get_vcpu_state() to
hide a valid snp_guest_vmsa_gpa whenever a deferred reload is pending?

Every path that clears or fails to install a guest-owned vmsa already sets
svm->sev_es.snp_guest_vmsa_gpa to INVALID_PAGE. The only case where
VALID_PAGE(svm->sev_es.snp_guest_vmsa_gpa) is true while
VALID_PAGE(svm->vmcb->control.vmsa_pa) is false is in
__sev_snp_reload_vmsa() when mmu_invalidate_retry_gfn() triggers a deferred
reload via KVM_REQ_VMSA_PAGE_RELOAD:

arch/x86/kvm/svm/sev.c:__sev_snp_reload_vmsa() {
	...
	svm->sev_es.snp_guest_vmsa_gpa = gpa;
	if (mmu_invalidate_retry_gfn(kvm, mmu_seq, gfn))
		kvm_make_request(KVM_REQ_VMSA_PAGE_RELOAD, vcpu);
	else
		svm->vmcb->control.vmsa_pa = pfn_to_hpa(pfn);
}

If the vCPU exits to userspace before vcpu_enter_guest() services
KVM_REQ_VMSA_PAGE_RELOAD, won't KVM_SEV_SNP_GET_VCPU_STATE omit
KVM_SEV_SNP_VCPU_STATE_VMSA_VALID and report vmsa_gpa as 0?

[Severity: Medium]
Conversely, what happens if the backing guest_memfd range or memslot is
invalidated while the vCPU is in userspace?

When sev_gmem_invalidate_range() runs, it reads snp_guest_vmsa_gpa via
READ_ONCE() and queues KVM_REQ_VMSA_PAGE_RELOAD without clearing
snp_guest_vmsa_gpa or vmsa_pa until the next KVM_RUN calls
sev_snp_reload_vmsa() in vcpu_enter_guest():

arch/x86/kvm/svm/sev.c:sev_gmem_invalidate_range() {
	...
	gpa_t gpa = READ_ONCE(to_svm(vcpu)->sev_es.snp_guest_vmsa_gpa);

	if (VALID_PAGE(gpa) &&
	    gpa_to_gfn(gpa) >= range->start &&
	    gpa_to_gfn(gpa) < range->end)
		kvm_make_request_and_kick(KVM_REQ_VMSA_PAGE_RELOAD, vcpu);
}

Because snp_get_vcpu_state() does not service pending
KVM_REQ_VMSA_PAGE_RELOAD (or KVM_REQ_UPDATE_PROTECTED_GUEST_STATE) requests,
won't KVM_SEV_SNP_GET_VCPU_STATE report stale vmsa state if called before
KVM_RUN?

Also, in snp_set_vcpu_state(), the else branch clears
svm->sev_es.snp_guest_vmsa_gpa = INVALID_PAGE outside mmu_lock without
WRITE_ONCE(), and neither branch clears a pending KVM_REQ_VMSA_PAGE_RELOAD
request. Should snp_set_vcpu_state() use WRITE_ONCE() and clear
KVM_REQ_VMSA_PAGE_RELOAD when updating or invalidating the vmsa?

> +
> +	if (VALID_PAGE(svm->vmcb->control.ghcb_gpa)) {
> +		state.ghcb_gpa = svm->vmcb->control.ghcb_gpa;
> +		state.valid_fields |= KVM_SEV_SNP_VCPU_STATE_GHCB_VALID;
> +	}
> +
> +	if (copy_to_user(u64_to_user_ptr(argp->data), &state, sizeof(state)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int snp_set_vcpu_state(struct kvm_vcpu *vcpu,
> +			      struct kvm_sev_cmd *argp)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct kvm *kvm = vcpu->kvm;
> +	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> +	struct kvm_sev_snp_vcpu_state state;
> +	int ret;
> +
> +	if (!is_sev_snp_guest(vcpu))
> +		return -ENOTTY;
> +	if (!sev->snp_direct_vmsa)
> +		return -EINVAL;
> +	if (!sev->snp_context || kvm->arch.pre_fault_allowed)
> +		return -EINVAL;

[Severity: High]
Does checking kvm->arch.pre_fault_allowed under vcpu->mutex actually
serialize snp_set_vcpu_state() against launch finalization?

In snp_launch_finish(), snp_launch_update_vmsa() locks all vCPUs, sets
vcpu->arch.guest_state_protected = true, and immediately unlocks all vCPUs
before snp_launch_finish() copies the user blobs, issues
SEV_CMD_SNP_LAUNCH_FINISH, and sets kvm->arch.pre_fault_allowed = true:

arch/x86/kvm/svm/sev.c:snp_launch_update_vmsa() {
	...
protect_vcpu:
		svm->vcpu.arch.guest_state_protected = true;
		svm_enable_lbrv(vcpu);
	}

out:
	kvm_unlock_all_vcpus(kvm);
	return ret;
}

Once kvm_unlock_all_vcpus() releases the vCPU mutexes (either while
SEV_CMD_SNP_LAUNCH_FINISH is in flight, or if snp_launch_finish() fails
before setting pre_fault_allowed), another thread calling
KVM_SEV_SNP_SET_VCPU_STATE can acquire vcpu->mutex, see
kvm->arch.pre_fault_allowed == false, and overwrite vmsa_pa,
snp_guest_vmsa_gpa, and ghcb_gpa on an already-protected vCPU.

Should snp_set_vcpu_state() check vcpu->arch.guest_state_protected instead?

> +
> +	if (copy_from_user(&state, u64_to_user_ptr(argp->data), sizeof(state)))
> +		return -EFAULT;
> +
> +	if (memchr_inv(state.pad, 0, sizeof(state.pad)) ||
> +	    state.valid_fields & ~(KVM_SEV_SNP_VCPU_STATE_VMSA_VALID |
> +				   KVM_SEV_SNP_VCPU_STATE_GHCB_VALID))
> +		return -EINVAL;
> +
> +	if (state.valid_fields & KVM_SEV_SNP_VCPU_STATE_VMSA_VALID) {
> +		if (!PAGE_ALIGNED(state.vmsa_gpa) ||
> +		    !page_address_valid(vcpu, state.vmsa_gpa) ||
> +		    IS_ALIGNED(state.vmsa_gpa, PMD_SIZE))
> +			return -EINVAL;
> +	}
> +
> +	guard(mutex)(&svm->sev_es.snp_vmsa_mutex);
> +
> +	if (state.valid_fields & KVM_SEV_SNP_VCPU_STATE_VMSA_VALID) {
> +		ret = sev_snp_install_guest_vmsa(svm, state.vmsa_gpa);
> +		if (ret)
> +			return ret;
> +	} else {
> +		svm->sev_es.snp_has_guest_vmsa = true;
> +		svm->sev_es.snp_guest_vmsa_gpa = INVALID_PAGE;
> +		svm->vmcb->control.vmsa_pa = INVALID_PAGE;
> +	}
> +
> +	if (state.valid_fields & KVM_SEV_SNP_VCPU_STATE_GHCB_VALID)
> +		svm->vmcb->control.ghcb_gpa = state.ghcb_gpa;
> +	else
> +		svm->vmcb->control.ghcb_gpa = INVALID_PAGE;

[Severity: High]
Does using INVALID_PAGE (~0ULL) for svm->vmcb->control.ghcb_gpa break GHCB
protocol handling and state reporting in snp_set_vcpu_state() and
snp_get_vcpu_state()?

svm->vmcb->control.ghcb_gpa backs MSR_AMD64_SEV_ES_GHCB, which is
initialized during vCPU reset in sev_es_init_vmcb() to the architectural
GHCB_MSR_SEV_INFO value:

arch/x86/kvm/svm/sev.c:sev_es_init_vmcb() {
	...
	if (!init_event)
		set_ghcb_msr(svm, GHCB_MSR_SEV_INFO((__u64)sev->ghcb_version,
						    GHCB_VERSION_MIN,
						    sev_enc_bit));
}

And sev_handle_vmgexit() inspects control->ghcb_gpa as follows:

arch/x86/kvm/svm/sev.c:sev_handle_vmgexit() {
	...
	ghcb_gpa = control->ghcb_gpa;
	if (ghcb_gpa & GHCB_MSR_INFO_MASK)
		return sev_handle_vmgexit_msr_protocol(svm);

	if (!ghcb_gpa) {
		...
	}

	if (is_sev_snp_guest(vcpu) &&
	    !ghcb_gpa_is_registered(svm, control->ghcb_gpa)) {
		...
		svm_vmgexit_bad_input(svm, GHCB_ERR_NOT_REGISTERED);
		return 1;
	}
}

This appears to cause three regressions:

1. When KVM_SEV_SNP_SET_VCPU_STATE is called without
KVM_SEV_SNP_VCPU_STATE_GHCB_VALID (for example, to set only the vmsa),
setting control.ghcb_gpa = INVALID_PAGE overwrites the GHCB_MSR_SEV_INFO
reset value with 0xffffffffffffffffULL, which has low 12 bits 0xfff and will
be misinterpreted by sev_handle_vmgexit() as a GHCB MSR protocol request.

2. When KVM_SEV_SNP_SET_VCPU_STATE is called with
KVM_SEV_SNP_VCPU_STATE_GHCB_VALID, snp_set_vcpu_state() writes
svm->vmcb->control.ghcb_gpa = state.ghcb_gpa without updating
svm->sev_es.ghcb_registered_gpa, so subsequent non-MSR VMGEXITs fail
ghcb_gpa_is_registered() with GHCB_ERR_NOT_REGISTERED.

3. In snp_get_vcpu_state(), VALID_PAGE(svm->vmcb->control.ghcb_gpa) only
tests != INVALID_PAGE, which evaluates to true for the initial
GHCB_MSR_SEV_INFO reset value, 0, and any GHCB MSR protocol value.

> +
> +	vmcb_mark_all_dirty(svm->vmcb);
> +	return 0;
> +}

[ ... ]

> @@ -4080,6 +4194,54 @@ static int snp_begin_psc(struct vcpu_svm *svm)
>  	return snp_do_psc(svm);
>  }
>  
> +/*
> + * Install a guest-owned VMSA.  The caller must serialize against AP creation
> + * and destruction with snp_vmsa_mutex.
> + */
> +static int sev_snp_install_guest_vmsa(struct vcpu_svm *svm, gpa_t gpa)
> +{
> +	struct kvm *kvm = svm->vcpu.kvm;
> +	struct kvm_memory_slot *slot;
> +	unsigned long mmu_seq;
> +	kvm_pfn_t pfn;
> +	gfn_t gfn;
> +	int idx;
> +	int ret;
> +
> +	lockdep_assert_held(&svm->sev_es.snp_vmsa_mutex);
> +
> +	gfn = gpa_to_gfn(gpa);
> +	idx = srcu_read_lock(&kvm->srcu);
> +	slot = gfn_to_memslot(kvm, gfn);
> +	if (!slot) {
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	mmu_seq = kvm->mmu_invalidate_seq;
> +	/* Pairs with the smp_wmb() in kvm_mmu_invalidate_end(). */
> +	smp_rmb();
> +
> +	/* Guest-owned VMSAs are backed by guest_memfd private memory. */
> +	ret = kvm_gmem_get_pfn(kvm, slot, gfn, &pfn, NULL);
> +	if (ret)
> +		goto out_unlock;

[Severity: High]
What happens if userspace calls KVM_SEV_SNP_SET_VCPU_STATE before populating
the vmsa page with KVM_SEV_SNP_LAUNCH_UPDATE?

In virt/kvm/guest_memfd.c, kvm_gmem_get_pfn() faults in an unpopulated folio
via FGP_CREAT and, if kvm_gmem_is_private_mem() is true, calls
kvm_arch_gmem_make_private() -> sev_gmem_make_private() ->
rmp_make_private(pfn, gpa, PG_LEVEL_4K, sev->asid, false):

virt/kvm/guest_memfd.c:kvm_gmem_get_pfn() {
	...
	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, max_order);
	...
	if (kvm_arch_has_gmem_convert() &&
	    kvm_gmem_is_private_mem(file_inode(file), index))
		r = kvm_arch_gmem_make_private(kvm, gfn, *pfn,
					       (kvm_pfn_t)1 << *max_order);
}

That transitions the unpopulated page's rmp entry to an unmeasured private
data page (Assigned=1, VMSA=0), causing a subsequent
KVM_SEV_SNP_LAUNCH_UPDATE on that gfn to fail in sev_gmem_post_populate()
with -EEXIST when snp_lookup_rmpentry() sees assigned == true.

In addition, kvm_gmem_get_pfn() returns -EFAULT if the memslot does not have
a guest_memfd (whereas !slot returns -EINVAL), and succeeds without error if
the gfn is currently marked shared in guest_memfd. Should
sev_snp_install_guest_vmsa() verify kvm_slot_has_gmem(slot),
kvm_mem_is_private(kvm, gfn), and that the page has already been populated as
a vmsa page rather than faulting it in via kvm_gmem_get_pfn()?

> +
> +	read_lock(&kvm->mmu_lock);
> +	if (mmu_invalidate_retry_gfn(kvm, mmu_seq, gfn)) {
> +		ret = -EAGAIN;
> +	} else {
> +		svm->sev_es.snp_has_guest_vmsa = true;
> +		WRITE_ONCE(svm->sev_es.snp_guest_vmsa_gpa, gpa);
> +		svm->vmcb->control.vmsa_pa = pfn_to_hpa(pfn);
> +	}

[Severity: High]
Does setting svm->vmcb->control.vmsa_pa = pfn_to_hpa(pfn) here allow
userspace to invoke KVM_RUN before KVM_SEV_SNP_LAUNCH_FINISH completes?

In direct-vmsa mode, KVM_SEV_SNP_LAUNCH_UPDATE with
KVM_SEV_SNP_PAGE_TYPE_VMSA already marks the page as a valid vmsa in the rmp.
Once sev_snp_install_guest_vmsa() sets svm->vmcb->control.vmsa_pa to a valid
address, neither sev_vcpu_needs_initialization() nor pre_sev_run() blocks
KVM_RUN before KVM_SEV_SNP_LAUNCH_FINISH:

arch/x86/kvm/svm/sev.c:sev_vcpu_needs_initialization() {
	return to_kvm_sev_info(vcpu->kvm)->need_init;
}

arch/x86/kvm/svm/sev.c:pre_sev_run() {
	...
	if (is_sev_es_guest(vcpu) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
		return -EINVAL;
}

Can userspace call KVM_RUN right after KVM_SEV_SNP_SET_VCPU_STATE and enter
the guest before snp_launch_finish() has set
vcpu->arch.guest_state_protected = true and enabled lbr virtualization?

[1] https://lore.kernel.org/r/20260908103338.427254-7-joro@8bytes.org

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