Re: [PATCH v1 1/1] KVM: PPC: Book3S: Protect TCE IOMMU table walks with RCU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: ppc: UAF of spapr_tce_table due to missing rcu_read_lock()
- [High] kvmppc_clear_tce: pinned memory leak on partial IOMMU map failure
--

commit b9a581a90633de9f36c89acf2c46a11e7fade7c3
Author: Narayana Murty N <nnmlinux@linux.ibm.com>

KVM: PPC: Book3S: Protect TCE IOMMU table walks with RCU

This commit replaces list_for_each_entry_lockless() with
list_for_each_entry_rcu() inside an rcu_read_lock() block for
stt->iommu_tables traversals in TCE hypercalls. This ensures that IOMMU
table entries removed concurrently are not reclaimed while the reader is
using them.

Cc: stable@vger.kernel.org

> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> index c5f3f5b29d89d..d8625488c3c5c 100644
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> @@ -602,7 +602,8 @@ long kvmppc_h_put_tce(struct kvm_vcpu *vcpu, unsigned long liobn,
>  
>  	entry = ioba >> stt->page_shift;
>  
> -	list_for_each_entry_lockless(stit, &stt->iommu_tables, next) {
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(stit, &stt->iommu_tables, next) {

[Severity: High]
Does this code leave the stt structure vulnerable to a use-after-free?

A guest VM can issue H_PUT_TCE or related hypercalls in preemptible virtual
mode. The handler calls kvmppc_find_table() to fetch the RCU-managed stt
pointer locklessly from the kvm->arch.spapr_tce_tables list. The handler
then dereferences stt multiple times before reaching this newly added
rcu_read_lock() block.

If a host userspace thread concurrently closes the kvm-spapr-tce file
descriptor, it triggers kvm_spapr_tce_release(), unlinking stt and queuing
it for deletion via call_rcu(). Because the hcall thread is preemptible and
holds no RCU read-side lock to protect the stt pointer itself, the RCU
grace period can expire and stt can be freed before the hcall finishes
using it here.

>  		if (dir == DMA_NONE)
>  			ret = kvmppc_tce_iommu_unmap(vcpu->kvm, stt,
>  					stit->tbl, entry);
> @@ -613,9 +614,11 @@ long kvmppc_h_put_tce(struct kvm_vcpu *vcpu, unsigned long liobn,
>  
>  		if (ret != H_SUCCESS) {
>  			kvmppc_clear_tce(vcpu->kvm->mm, stt, stit->tbl, entry);

[Severity: High]
Can this error path cause a permanent leak of host pinned memory?

If kvmppc_tce_iommu_map() fails partway through the stt->iommu_tables list,
the code calls kvmppc_clear_tce() on the failing table. kvmppc_clear_tce()
unmaps the hardware TCE using iommu_tce_xchg_no_kill(), but it discards the
old dir and hpa values without dropping the mm_iommu_mapped pin count:

kvmppc_clear_tce() {
    ...
    for (i = 0; i < subpages; ++i) {
        unsigned long hpa = 0;
        enum dma_data_direction dir = DMA_NONE;

        iommu_tce_xchg_no_kill(mm, tbl, io_entry + i, &hpa, &dir);
    }
}

Because the hardware entry is now DMA_NONE, future guest unmaps or VFIO
container teardowns will skip decrementing the pin count. Furthermore,
previously mapped tables in the list are left mapped without being rolled
back, causing hardware inconsistency and further silent memory leaks when
the guest abandons the failed mapping.

> +			rcu_read_unlock();
>  			goto unlock_exit;
>  		}
>  	}
> +	rcu_read_unlock();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015120137.28843-1-nnmlinux@linux.ibm.com?part=1