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

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

[Narayana Murty N]

Commit cad32d9d42e8 ("KVM: PPC: Book3s: Retire H_PUT_TCE/etc real mode
handlers") moved the TCE hcalls from real mode to preemptible virtual
mode.

The stt->iommu_tables list is RCU managed. Entries are added with
list_add_rcu(), removed with list_del_rcu(), and released after an RCU
grace period with call_rcu().

However, kvmppc_h_put_tce(), kvmppc_h_put_tce_indirect(), and
kvmppc_h_stuff_tce() traverse iommu_tables using
list_for_each_entry_lockless() without an RCU read-side critical
section.

A concurrent IOMMU table detach can therefore remove an entry while a
TCE hcall is traversing or using it, without the traversal preventing
the entry from being reclaimed.

Use list_for_each_entry_rcu() under rcu_read_lock() for these walks so
that an entry removed concurrently is not reclaimed until the reader
has completed.

Fixes: cad32d9d42e8 ("KVM: PPC: Book3s: Retire H_PUT_TCE/etc real mode handlers")
Cc: stable@vger.kernel.org
Signed-off-by: Narayana Murty N <nnmlinux@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_64_vio.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
index c5f3f5b29d89..d8625488c3c5 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) {
 		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);
+			rcu_read_unlock();
 			goto unlock_exit;
 		}
 	}
+	rcu_read_unlock();
 
 	kvmppc_tce_put(stt, entry, tce);
 
@@ -697,7 +700,8 @@ long kvmppc_h_put_tce_indirect(struct kvm_vcpu *vcpu,
 			goto unlock_exit;
 		}
 
-		list_for_each_entry_lockless(stit, &stt->iommu_tables, next) {
+		rcu_read_lock();
+		list_for_each_entry_rcu(stit, &stt->iommu_tables, next) {
 			ret = kvmppc_tce_iommu_map(vcpu->kvm, stt,
 					stit->tbl, entry + i, ua,
 					iommu_tce_direction(tce));
@@ -705,9 +709,11 @@ long kvmppc_h_put_tce_indirect(struct kvm_vcpu *vcpu,
 			if (ret != H_SUCCESS) {
 				kvmppc_clear_tce(vcpu->kvm->mm, stt, stit->tbl,
 						 entry + i);
+				rcu_read_unlock();
 				goto unlock_exit;
 			}
 		}
+		rcu_read_unlock();
 
 		kvmppc_tce_put(stt, entry + i, tce);
 	}
@@ -739,7 +745,8 @@ long kvmppc_h_stuff_tce(struct kvm_vcpu *vcpu,
 	if (tce_value & (TCE_PCI_WRITE | TCE_PCI_READ))
 		return H_PARAMETER;
 
-	list_for_each_entry_lockless(stit, &stt->iommu_tables, next) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(stit, &stt->iommu_tables, next) {
 		unsigned long entry = ioba >> stt->page_shift;
 
 		for (i = 0; i < npages; ++i) {
@@ -749,13 +756,16 @@ long kvmppc_h_stuff_tce(struct kvm_vcpu *vcpu,
 			if (ret == H_SUCCESS)
 				continue;
 
-			if (ret == H_TOO_HARD)
+			if (ret == H_TOO_HARD) {
+				rcu_read_unlock();
 				return ret;
+			}
 
 			WARN_ON_ONCE(1);
 			kvmppc_clear_tce(vcpu->kvm->mm, stt, stit->tbl, entry + i);
 		}
 	}
+	rcu_read_unlock();
 
 	for (i = 0; i < npages; ++i, ioba += (1ULL << stt->page_shift))
 		kvmppc_tce_put(stt, ioba >> stt->page_shift, tce_value);
-- 
2.54.0

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

[sashiko-bot]

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