[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

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

[Ritesh Harjani]

Hi Narayana,

Narayana Murty N <nnmlinux@linux.ibm.com> writes:

> 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")

I am not sure if this is the right fix. On doing git blame, I see the
following patch introduced this:

121f80ba ("KVM: PPC: VFIO: Add in-kernel acceleration for VFIO") - Alexey Kardashevskiy


> 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(-)


Sashiko pointed out few more bugs around this path:

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

So, even though these looks to be pre-existing bugs - do you think we
can address them in a single patch series itself? This will reduce your
testing/validation as well?
Also since we are anyway fixing RCU specific bugs in patch-1, so I think
we can also fix rcu handling for stt as well?

Do you mind working on v2 and addressing Sashiko review comments please?

-ritesh