lkml.org 
[lkml]   [2022]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
    Hi, Dave and Jean,

    On Tue, Apr 26, 2022 at 01:04:45PM +0800, Zhangfei Gao wrote:
    >
    >
    > On 2022/4/26 下午12:20, Fenghua Yu wrote:
    > > Hi, Jean and Zhangfei,
    > >
    > > On Mon, Apr 25, 2022 at 05:13:02PM +0100, Jean-Philippe Brucker wrote:
    > > > Could we move mm_pasid_drop() to __mmdrop() instead of __mmput()? For Arm
    > > > we do need to hold the mm_count until unbind(), and mmgrab()/mmdrop() is
    > > > also part of Lu's rework [1].
    > > Is this a right fix for the issue? Could you please test it on ARM?
    > > I don't have an ARM machine.
    > >
    > > Thanks.
    > >
    > > -Fenghua
    > >
    > > From 84aa68f6174439d863c40cdc2db0e1b89d620dd0 Mon Sep 17 00:00:00 2001
    > > From: Fenghua Yu <fenghua.yu@intel.com>
    > > Date: Fri, 15 Apr 2022 00:51:33 -0700
    > > Subject: [PATCH] iommu/sva: Fix PASID use-after-free issue
    > >
    > > A PASID might be still used on ARM after it is freed in __mmput().
    > >
    > > process:
    > > open()->sva_bind()->ioasid_alloc() = N; // Get PASID N for the mm
    > > exit();
    > > exit_mm()->__mmput()->mm_pasid_drop()->mm->pasid = -1; // PASID -1
    > > exit_files()->release(dev)->sva_unbind()->use mm->pasid; // Failure
    > >
    > > To avoid the use-after-free issue, free the PASID after no device uses it,
    > > i.e. after all devices are unbound from the mm.
    > >
    > > sva_bind()/sva_unbind() call mmgrab()/mmdrop() to track mm->mm_count.
    > > __mmdrop() is called only after mm->mm_count is zero. So freeing the PASID
    > > in __mmdrop() guarantees the PASID is safely freed only after no device
    > > is bound to the mm.
    > >
    > > Fixes: 701fac40384f ("iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit")
    > >
    > > Reported-by: Zhangfei Gao <zhangfei.gao@foxmail.com>
    > > Suggested-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
    > > Suggested-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
    > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
    > Thanks for the fix.
    >
    > Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org>
    >
    >
    > > ---
    > > 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);
    > > }
    >

    Is this patch a good fix? Will you help push the fix into upstream?

    Thank you very much!

    -Fenghua

    \
     
     \ /
      Last update: 2022-04-28 02:55    [W:4.068 / U:0.776 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site