Re: [PATCH 09/11] KVM: arm64: CCA: Prevent memslot modifications after Realm creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: arm64: CCA: guest_memfd memslot delete rejected after Stage-2 unmap
--

commit 5106402397c36eec1e5fa59a5bb2c5adf4ee2aa8
Author: Suzuki K Poulose <suzuki.poulose@arm.com>

KVM: arm64: CCA: Prevent memslot modifications after Realm creation

This commit updates kvm_arch_prepare_memory_region() to disallow creating,
modifying, or deleting guest_memfd-backed memslots once a Realm has been
created.

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 51fda7b2ff7f3..eb8c5f9e9791d 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -2862,13 +2862,28 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  	    (new->flags & (KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY)))
>  		return -EPERM;
>  
> -	if (kvm->arch.vm_flavor == VM_PROTECTED_PKVM) {
> +	switch (kvm->arch.vm_flavor) {
> +	case VM_PROTECTED_PKVM:
>  		/* Cannot modify memslots once a pVM has run. */
>  		if (pkvm_hyp_vm_is_created(kvm) &&
> -		    (change == KVM_MR_DELETE || change == KVM_MR_MOVE)) {
> +		    (change == KVM_MR_DELETE || change == KVM_MR_MOVE))
>  			return -EPERM;
> -		}
> +		break;
> +	case VM_REALM:
> +		/*
> +		 * Once the Realm is created, we cannot modify any slots that
> +		 * could be providing private memory. i.e., guest_memfd backed
> +		 * slots.
> +		 * TODO: Handle trusted device private memory slots
> +		 */
> +		if (!kvm_realm_is_created(kvm))
> +			break;
>  
> +		if (kvm_slot_has_gmem(old) || kvm_slot_has_gmem(new))
> +			return -EPERM;

[Severity: High]
Does checking kvm_slot_has_gmem(old) in kvm_arch_prepare_memory_region() run
too late to prevent Stage-2 unmapping when deleting a guest_memfd-backed
memslot on a created Realm?

In virt/kvm/kvm_main.c:kvm_set_memory_region(), non-delete changes
(KVM_MR_MOVE and KVM_MR_FLAGS_ONLY) on KVM_MEM_GUEST_MEMFD memslots are
already rejected upfront with -EINVAL, so kvm_slot_has_gmem(old) is reached
here when change == KVM_MR_DELETE.

However, in virt/kvm/kvm_main.c:kvm_set_memslot(), KVM invalidates and
flushes the old memslot before calling kvm_prepare_memory_region():

virt/kvm/kvm_main.c:kvm_set_memslot() {
	if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) {
		invalid_slot = kzalloc_obj(*invalid_slot, GFP_KERNEL_ACCOUNT);
		if (!invalid_slot) {
			mutex_unlock(&kvm->slots_arch_lock);
			return -ENOMEM;
		}
		kvm_invalidate_memslot(kvm, old, invalid_slot);
	}

	r = kvm_prepare_memory_region(kvm, old, new, change);
	if (r)
		goto err;
}

kvm_invalidate_memslot() marks the active slot as KVM_MEMSLOT_INVALID and
calls arch/arm64/kvm/mmu.c:kvm_arch_flush_shadow_memslot():

arch/arm64/kvm/mmu.c:kvm_arch_flush_shadow_memslot() {
	write_lock(&kvm->mmu_lock);
	kvm_stage2_unmap_range(&kvm->arch.mmu, gpa, size, true);
	kvm_nested_unmap_cipa_range(kvm, gpa, size, true);
	write_unlock(&kvm->mmu_lock);
}

For a Realm VM, kvm_stage2_unmap_range() invokes
arch/arm64/kvm/rmi.c:kvm_realm_unmap_range_filter() with
KVM_FILTER_PRIVATE | KVM_FILTER_SHARED:

arch/arm64/kvm/rmi.c:kvm_realm_unmap_range_filter() {
	if (attr_filter & KVM_FILTER_SHARED)
		realm_unmap_shared_range(kvm, start, end, may_block);
	if (attr_filter & KVM_FILTER_PRIVATE)
		realm_unmap_private_range(kvm, start, end, may_block);
}

By the time kvm_arch_prepare_memory_region() returns -EPERM and
kvm_set_memslot() restores the old memslot in the error path, hasn't
realm_unmap_private_range() already unmapped and undelegated the Realm's
private Stage-2 DATA granules across the memslot's GPA range?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102141304.126883-1-steven.price@arm.com?part=9