Re: [PATCH 07/11] KVM: arm64: CCA: Tear down RTTs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] KVM: arm64: stage-2 PGD leak for uncreated Realms in kvm_destroy_realm()
- [High] KVM: arm64: unclamped stage-2 levels in get_start_level() breaks 64K RTT teardown
- [High] KVM: arm64: free_rtt() accounts page table pages after freeing the page
- [Medium] KVM: arm64: kvm_realm_uninit_stage2() ignores REALM_STATE_NEW
--

commit 5954ac2ccf8825a659e2f39dc0db05d96c40be84
Author: Steven Price <steven.price@arm.com>

KVM: arm64: CCA: Tear down RTTs

This commit adds stage-2 page table and Realm Translation Table (RTT)
teardown for arm64 CCA realms. It unmaps the Realm IPA space during stage-2
teardown and walks non-root RTTs during final Realm destruction to destroy
and undelegate them before destroying the Realm Descriptor.

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index b049ea67d9af3..51fda7b2ff7f3 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1145,9 +1145,24 @@ int kvm_init_stage2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu, unsigned long t
>  	return err;
>  }
>  
> +static void kvm_realm_uninit_stage2(struct kvm_s2_mmu *mmu)
> +{
> +	struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
> +
> +	mutex_lock(&kvm->arch.config_lock);
> +	if (kvm_realm_state(kvm) == REALM_STATE_ACTIVE) {
> +		kvm_set_realm_state(kvm, REALM_STATE_DYING);
> +		WARN_ON(kvm_realm_teardown_stage2(kvm));
> +	}
> +	mutex_unlock(&kvm->arch.config_lock);
> +}

[Severity: Medium]
What happens if kvm_uninit_stage2_mmu() is called via
kvm_mmu_notifier_release() -> kvm_flush_shadow_all() ->
kvm_arch_flush_shadow_all() while the Realm is in REALM_STATE_NEW?

According to enum realm_state in arch/arm64/include/asm/kvm_rmi.h, the Realm
Descriptor has already been created and stage-2 memory may already be
populated in REALM_STATE_NEW. Unlike kvm_destroy_realm(), which checks
kvm_realm_is_created(kvm), sets REALM_STATE_DYING, and unmaps stage-2 for
both REALM_STATE_NEW and REALM_STATE_ACTIVE, kvm_realm_uninit_stage2() only
checks for REALM_STATE_ACTIVE.

Should kvm_realm_uninit_stage2() also handle REALM_STATE_NEW (for example by
checking kvm_realm_is_created(kvm)) so that stage-2 mappings are unmapped
before exit_mmap() finishes and the Realm transitions to REALM_STATE_DYING?

>  void kvm_uninit_stage2_mmu(struct kvm *kvm)
>  {
> -	kvm_free_stage2_pgd(&kvm->arch.mmu);
> +	if (kvm_vm_is_realm(kvm))
> +		kvm_realm_uninit_stage2(&kvm->arch.mmu);
> +	else
> +		kvm_free_stage2_pgd(&kvm->arch.mmu);
>  	kvm_mmu_free_memory_cache(&kvm->arch.mmu.split_page_cache);
>  }

[Severity: High]
Does skipping kvm_free_stage2_pgd() here leak mmu->pgt, the root stage-2 PGD
pages (mmu->pgt->pgd), and the per-CPU mmu->last_vcpu_ran allocation when a
Realm VM is destroyed before the Realm is created in the RMM?

During VM creation, kvm_arch_init_vm() calls kvm_init_stage2_mmu() while the
Realm state is still REALM_STATE_NONE. If userspace closes the VM file
descriptor without creating the Realm in the RMM, kvm_destroy_realm() takes
its early return before reaching kvm_free_stage2_pgd(&kvm->arch.mmu):

arch/arm64/kvm/rmi.c:kvm_destroy_realm() {
	...
	if (!kvm_realm_is_created(kvm)) {
		kfree(realm->sro);
		realm->sro = NULL;
		return;
	}
	...
	kvm_free_stage2_pgd(&kvm->arch.mmu);
}

Similarly, if kvm_init_nested() or kvm_init_realm() fails in
kvm_arch_init_vm() after kvm_init_stage2_mmu() succeeds:

arch/arm64/kvm/arm.c:kvm_arch_init_vm() {
	...
	ret = kvm_init_nested(kvm);
	if (ret)
		goto err_uninit_mmu;

	if (kvm_vm_is_realm(kvm)) {
		ret = kvm_init_realm(kvm);
		if (ret)
			goto err_uninit_mmu;
	}

	return 0;

err_uninit_mmu:
	kvm_uninit_stage2_mmu(kvm);
	return ret;
}

the error path only calls kvm_uninit_stage2_mmu(kvm) and never calls
kvm_destroy_realm(), leaking the stage-2 PGD allocations there as well.

> diff --git a/arch/arm64/kvm/rmi.c b/arch/arm64/kvm/rmi.c
> index 45ecbaa467d1d..f190143b33ba5 100644
> --- a/arch/arm64/kvm/rmi.c
> +++ b/arch/arm64/kvm/rmi.c

[ ... ]

> @@ -21,11 +29,180 @@ u32 kvm_rmm_ipa_limit(void)
>  	return u64_get_bits(rmi_feat_reg(0), RMI_FEATURE_REGISTER_0_S2SZ);
>  }
>  
> +static int get_start_level(struct realm *realm)
> +{
> +	return 4 - stage2_pgtable_levels(realm->ia_bits);
> +}

[Severity: High]
Can get_start_level() return a level that mismatches the stage-2 page table
configuration on 64KB page kernels and cause realm_destroy_rtts() to fail?

When kvm_init_ipa_range() configures mmu->vtcr via kvm_get_vtcr(), the
number of stage-2 levels is clamped to at least 2:

arch/arm64/kvm/hyp/pgtable.c:kvm_get_vtcr() {
	...
	lvls = stage2_pgtable_levels(phys_shift);
	if (lvls < 2)
		lvls = 2;
	...
}

Under CONFIG_ARM64_64K_PAGES with a 32-bit or 33-bit IPA,
stage2_pgtable_levels(realm->ia_bits) evaluates to 1, so get_start_level()
returns 3 (KVM_PGTABLE_LAST_LEVEL) instead of 2.

When kvm_destroy_realm() -> realm_destroy_rtts() ->
realm_tear_down_rtt_range() then calls:

	realm_tear_down_rtt_level(realm, get_start_level(realm) + 1,
				  start, end);

it passes level 4 into realm_tear_down_rtt_level(), hitting
WARN_ON(level > KVM_PGTABLE_LAST_LEVEL) and returning -EINVAL. That in turn
causes kvm_destroy_realm() to warn and return early, leaking the delegated
RD, RTT granules, root stage-2 PGD pages, and realm->sro.

> +
> +static void free_rtt(phys_addr_t phys)
> +{
> +	if (rmi_free_delegated_page(phys))
> +		return;
> +
> +	kvm_account_pgtable_pages(phys_to_virt(phys), -1);
> +}

[Severity: High]
Does calling kvm_account_pgtable_pages() after rmi_free_delegated_page()
access the page after it has already been freed and uncharged from its
memcg?

In drivers/firmware/arm_rmm/rmi.c, rmi_free_delegated_page() undelegates the
granule and immediately frees the page back to the page allocator:

drivers/firmware/arm_rmm/rmi.c:rmi_free_delegated_page() {
	if (WARN_ON_ONCE(rmi_undelegate_page(phys))) {
		/* Undelegate failed: leak the page */
		return -EBUSY;
	}

	free_page((unsigned long)phys_to_virt(phys));

	return 0;
}

Because free_page() clears page->memcg_data in __free_pages_prepare() and
releases the page for reallocation, the subsequent call to
kvm_account_pgtable_pages(phys_to_virt(phys), -1) ->
mod_lruvec_page_state() operates on a freed struct page and sees a NULL
folio_memcg(), failing to decrement the memory cgroup's
NR_SECONDARY_PAGETABLE counter (or racing with a concurrent reallocation of
that page).

Should kvm_account_pgtable_pages(phys_to_virt(phys), -1) be called before
freeing the page, matching kvm_s2_free_pages_exact() and kvm_s2_put_page()
in arch/arm64/kvm/mmu.c?

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