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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] KVM: arm64: possible skipped parent retry in
  realm_tear_down_rtt_level()
- [Low] KVM: arm64: cca: leak of realm->sro on error returns in
  kvm_destroy_realm()
--

commit 9b5e6ebefa08ce4aff4f7ddec8f9b62647e79039
Author: Steven Price <steven.price@arm.com>

KVM: arm64: CCA: Tear down RTTs

This commit adds stage-2 unmapping and RTT destruction for CCA Realms,
walking and undelegating non-root RTTs before destroying the Realm
Descriptor.

> diff --git a/arch/arm64/kvm/rmi.c b/arch/arm64/kvm/rmi.c
> index 69c628cfb6c74..b904e6541fb07 100644
> --- a/arch/arm64/kvm/rmi.c
> +++ b/arch/arm64/kvm/rmi.c

[ ... ]

> +static int realm_tear_down_rtt_level(struct realm *realm, int level,
> +				     unsigned long start, unsigned long end)
> +{
> +	ssize_t map_size;
> +	unsigned long addr, next_addr;
> +
> +	if (WARN_ON(level > KVM_PGTABLE_LAST_LEVEL))
> +		return -EINVAL;
> +
> +	map_size = rmi_rtt_level_mapsize(level - 1);
> +
> +	for (addr = start; addr < end; addr = next_addr) {
> +		phys_addr_t rtt_granule;
> +		long ret;
> +		unsigned long align_addr = ALIGN(addr, map_size);
> +
> +		next_addr = ALIGN(addr + 1, map_size);
> +
> +		if (next_addr > end || align_addr != addr) {
> +			/*
> +			 * The target range is smaller than what this level
> +			 * covers, recurse deeper.
> +			 */
> +			ret = realm_tear_down_rtt_level(realm,
> +							level + 1,
> +							addr,
> +							min(next_addr, end));
> +			if (ret)
> +				return ret;
> +			continue;
> +		}
> +
> +		ret = realm_rtt_destroy(realm, addr, level,
> +					&rtt_granule, &next_addr);
> +		if (ret < 0)
> +			return ret;
> +
> +		switch (RMI_RESULT_STATUS(ret)) {
> +		case RMI_SUCCESS:
> +			free_rtt(rtt_granule);
> +			break;
> +		case RMI_ERROR_RTT:
> +			if (next_addr > addr) {
> +				/* Missing RTT, skip */
> +				break;
> +			}
> +			/*
> +			 * We tear down the RTT range for the full IPA
> +			 * space, after everything is unmapped. Also we
> +			 * descend down only if we cannot tear down a
> +			 * top level RTT. Thus RMM must be able to walk
> +			 * to the requested level. e.g., a block mapping
> +			 * exists at L1 or L2.
> +			 */
> +			if (WARN_ON(RMI_RESULT_DATA_LEVEL(ret) != level))
> +				return -EBUSY;

[Severity: Low]
Can SMC_RMI_RTT_DESTROY return RMI_ERROR_RTT with next_addr > addr (and
next_addr < addr + map_size) when the target RTT at level has non-live
initial entries followed by a live child table entry, or does the walk
terminate at level - 1?  This rests on an RMM specification detail for
SMC_RMI_RTT_DESTROY that could not be confirmed from the kernel source.

If RMM returns RMI_RESULT_DATA_LEVEL(ret) == level with addr < next_addr <
addr + map_size, the next_addr > addr check treats the live RTT as missing
and advances addr to the unaligned next_addr.  The next iteration sees
align_addr != addr, tears down the remaining child RTTs at level + 1, and
executes continue, advancing to the next map_size boundary without retrying
realm_rtt_destroy() on the parent RTT at level.  Alternatively, if the walk
terminates at the parent entry and returns RMI_RESULT_DATA_LEVEL(ret) ==
level - 1 with next_addr == addr, WARN_ON(RMI_RESULT_DATA_LEVEL(ret) !=
level) triggers and aborts teardown with -EBUSY, leaving the RD and RTTs
leaked.

[ ... ]

> @@ -29,12 +206,24 @@ void kvm_destroy_realm(struct kvm *kvm)
>  
>  	kvm_set_realm_state(kvm, REALM_STATE_DYING);
>  
> +	/*
> +	 * REALM_DESTROY requires the realm to be non-live: all RECs must have
> +	 * been destroyed and the root RTTs must be empty. Unmap the IPA space
> +	 * and destroy any non-root RTTs before tearing down the RD. The root
> +	 * RTT pages are still owned by the RMM at this point, so keep the KVM
> +	 * pgtable alive until after REALM_DESTROY and undelegation.
> +	 */
> +	realm_unmap_stage2(kvm);
> +
>  	if (realm->rd) {
>  		phys_addr_t rd_phys = virt_to_phys(realm->rd);
>  
>  		if (WARN_ON(rmi_realm_terminate(rd_phys, realm->sro)))
>  			return;
>  
> +		if (WARN_ON(realm_destroy_rtts(kvm)))
> +			return;
> +
>  		if (WARN_ON(rmi_realm_destroy(rd_phys, realm->sro)))
>  			return;
>  		rmi_free_delegated_page(rd_phys);

[Severity: Low]
Does returning early on failure in realm_destroy_rtts() (or the surrounding
RMI teardown calls) leak realm->sro?  Unlike realm->rd and the stage-2 PGD
pages, realm->sro is host-owned heap memory that is never delegated to the
RMM, and kfree(realm->sro) at the end of kvm_destroy_realm() is skipped on
these early returns.

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