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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Narayana Murty N <nnmlinux@xxxxxxxxxxxxx>
> > ---
> >   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