Re: [PATCH v2] iommu/sva: Fix PASID use-after-free issue

[Zhangfei Gao]

On 2022/4/29 上午9:39, Zhangfei Gao wrote:
> On 2022/4/29 上午2:00, Fenghua Yu wrote:
> > The PASID is being freed too early.  It needs to stay around until after
> > device drivers that might be using it have had a chance to clear it out
> > of the hardware.
> >
> > As a reminder:
> >
> > mmget() /mmput()  refcount the mm's address space
> > mmgrab()/mmdrop() refcount the mm itself
> >
> > The PASID is currently tied to the life of the mm's address space and
> > freed in __mmput().  This makes logical sense because the PASID can't be
> > used once the address space is gone.
> >
> > But, this misses an important point: even after the address space is
> > gone, the PASID will still be programmed into a device.  Device drivers
> > might, for instance, still need to flush operations that are outstanding
> > and need to use that PASID.  They do this at file->release() time.
> >
> > Device drivers call the IOMMU driver to hold a reference on the mm 
> > itself
> > and drop it at file->release() time.  But, the IOMMU driver holds a
> > reference on the mm itself, not the address space.  The address space
> > (and the PASID) is long gone by the time the driver tries to clean up.
> > This is effectively a use-after-free bug on the PASID.
> >
> > To fix this, move the PASID free operation from __mmput() to __mmdrop().
> > This ensures that the IOMMU driver's existing mmgrab() keeps the PASID
> > allocated until it drops its mm reference.
> >
> > Fixes: 701fac40384f ("iommu/sva: Assign a PASID to mm on PASID 
> > allocation and free it on mm exit")
> >
> > Reported-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxxx>
> > Tested-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxxx>
>
> Tested-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
>
> Use the formal email, thanks
>
> > Suggested-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> > Suggested-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > Reviewed-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> > Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>

Hi,

Will this be taken for 5.18?

Thanks

> > ---
> >
> > v2:
> > - Dave Hansen rewrites the change log.
> > - Add Tested-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxxx>
> > - Add Reviewed-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> >
> > The original patch was posted and discussed in:
> > https://lore.kernel.org/lkml/YmdzFFx7fN586jcf@xxxxxxxxxxxxxxxxx/
> >
> >   kernel/fork.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 9796897560ab..35a3beff140b 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -792,6 +792,7 @@ void __mmdrop(struct mm_struct *mm)
> >       mmu_notifier_subscriptions_destroy(mm);
> >       check_mm(mm);
> >       put_user_ns(mm->user_ns);
> > +    mm_pasid_drop(mm);
> >       free_mm(mm);
> >   }
> >   EXPORT_SYMBOL_GPL(__mmdrop);
> > @@ -1190,7 +1191,6 @@ static inline void __mmput(struct mm_struct *mm)
> >       }
> >       if (mm->binfmt)
> >           module_put(mm->binfmt->module);
> > -    mm_pasid_drop(mm);
> >       mmdrop(mm);
> >   }