[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

[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

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

[Narayana Murty N]

Hi Ritesh,

Thanks for the review.

On 30/10/26 7:32 PM, Ritesh Harjani (IBM) wrote:
> 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
> 
Regarding the Fixes: tag, thanks for pointing this out. I will re-check 
the history around 121f80ba ("KVM: PPC: VFIO: Add in-kernel acceleration 
for VFIO") and update the commit message/Fixes tag accordingly, since 
that appears to be where the lockless iommu_tables traversal was 
originally introduced rather than cad32d9d42e8.
> 
>> 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?
Yes, I will address the Sashiko findings in the next version. I will 
include the stt lifetime/RCU handling along with the existing 
stt->iommu_tables RCU fix, and handle the partial IOMMU map 
failure/pinned-memory leak as part of the same series.

I'll rework this as v2 addressing these points.
> 
> -ritesh
> 
>