Messages in this thread | | | Subject | Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit | From | "" <> | Date | Thu, 21 Apr 2022 14:47:23 +0800 |
| |
On 2022/4/21 上午12:45, Jean-Philippe Brucker wrote: > Hi, > > On Fri, Apr 15, 2022 at 02:51:08AM -0700, Fenghua Yu wrote: >> From a6444e1e5bd8076f5e5c5e950d3192de327f0c9c Mon Sep 17 00:00:00 2001 >> From: Fenghua Yu <fenghua.yu@intel.com> >> Date: Fri, 15 Apr 2022 00:51:33 -0700 >> Subject: [RFC PATCH] iommu/sva: Fix PASID use-after-free issue >> >> A PASID might be still used even though it is freed on mm exit. >> >> process A: >> sva_bind(); >> ioasid_alloc() = N; // Get PASID N for the mm >> fork(): // spawn process B >> exit(); >> ioasid_free(N); >> >> process B: >> device uses PASID N -> failure >> sva_unbind(); >> >> Dave Hansen suggests to take a refcount on the mm whenever binding the >> PASID to a device and drop the refcount on unbinding. The mm won't be >> dropped if the PASID is still bound to it. >> >> 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: Dave Hansen" <dave.hansen@linux.intel.com> >> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> >> --- >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 6 ++++++ >> drivers/iommu/intel/svm.c | 4 ++++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c >> index 22ddd05bbdcd..3fcb842a0df0 100644 >> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c >> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c >> @@ -7,6 +7,7 @@ >> #include <linux/mmu_context.h> >> #include <linux/mmu_notifier.h> >> #include <linux/slab.h> >> +#include <linux/sched/mm.h> >> >> #include "arm-smmu-v3.h" >> #include "../../iommu-sva-lib.h" >> @@ -363,6 +364,9 @@ arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata) >> >> mutex_lock(&sva_lock); >> handle = __arm_smmu_sva_bind(dev, mm); >> + /* Take an mm refcount on a successful bind. */ >> + if (!IS_ERR(handle)) >> + mmget(mm); >> mutex_unlock(&sva_lock); >> return handle; >> } >> @@ -372,6 +376,8 @@ void arm_smmu_sva_unbind(struct iommu_sva *handle) >> struct arm_smmu_bond *bond = sva_to_bond(handle); >> >> mutex_lock(&sva_lock); >> + /* Drop an mm refcount. */ >> + mmput(bond->mm); > I do like the idea because it will simplify the driver. We can't call > mmput() here, though, because it may call the release() MMU notifier which > will try to grab sva_lock, already held. > > I also found another use-after-free in arm_smmu_free_shared_cd(), where we > call arm64_mm_context_put() when the mm could already be freed. There used > to be an mmgrab() preventing this but it went away during a rewrite. > > To fix both we could just move mmput() at the end of unbind() but I'd > rather do a proper cleanup removing the release() notifier right away. > Zhangfei, could you try the patch below? > > Thanks, > Jean > > --- 8< --- > > From 4e09c0d71dfb35fc90915bd1e36545027fbf8a03 Mon Sep 17 00:00:00 2001 > From: Jean-Philippe Brucker <jean-philippe@linaro.org> > Date: Wed, 20 Apr 2022 10:19:24 +0100 > Subject: [PATCH] iommu/arm-smmu-v3-sva: Fix PASID and mm use-after-free issues > > Commit 701fac40384f ("iommu/sva: Assign a PASID to mm on PASID > allocation and free it on mm exit") frees the PASID earlier than what > the SMMUv3 driver expects. At the moment the SMMU driver handles mm exit > in the release() MMU notifier by quiescing the context descriptor. The > context descriptor is only made invalid in unbind(), after the device > driver ensured the PASID is not used anymore. Releasing the PASID on mm > exit may cause it to be reallocated while it is still used by the > context descriptor. > > There is another use-after-free, present since the beginning, where we > call arm64_mm_context_put() without a guarantee that mm_count is held. > > Dave Hansen suggests to grab mm_users whenever binding the mm to a > device and drop it on unbinding. With that we can fix both issues and > simplify the driver by removing the release() notifier. > > Fixes: 32784a9562fb ("iommu/arm-smmu-v3: Implement iommu_sva_bind/unbind()") > Reported-by: Zhangfei Gao <zhangfei.gao@foxmail.com> > Suggested-by: Dave Hansen <dave.hansen@linux.intel.com> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 49 +++++-------------- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 14 +----- > 3 files changed, 15 insertions(+), 49 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > index cd48590ada30..d50d215d946c 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -735,7 +735,6 @@ static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) > > extern struct xarray arm_smmu_asid_xa; > extern struct mutex arm_smmu_asid_lock; > -extern struct arm_smmu_ctx_desc quiet_cd; > > int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid, > struct arm_smmu_ctx_desc *cd); > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > index 22ddd05bbdcd..f9dff0f6cdd4 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > @@ -6,6 +6,7 @@ > #include <linux/mm.h> > #include <linux/mmu_context.h> > #include <linux/mmu_notifier.h> > +#include <linux/sched/mm.h> > #include <linux/slab.h> > > #include "arm-smmu-v3.h" > @@ -15,7 +16,6 @@ > struct arm_smmu_mmu_notifier { > struct mmu_notifier mn; > struct arm_smmu_ctx_desc *cd; > - bool cleared; > refcount_t refs; > struct list_head list; > struct arm_smmu_domain *domain; > @@ -96,9 +96,14 @@ static struct arm_smmu_ctx_desc *arm_smmu_alloc_shared_cd(struct mm_struct *mm) > struct arm_smmu_ctx_desc *cd; > struct arm_smmu_ctx_desc *ret = NULL; > > + /* Prevent mm exit as long as it is used by the context descriptor */ > + mmget(mm); > + > asid = arm64_mm_context_get(mm); > - if (!asid) > - return ERR_PTR(-ESRCH); > + if (!asid) { > + err = -ESRCH; > + goto out_put_mm; > + } > > cd = kzalloc(sizeof(*cd), GFP_KERNEL); > if (!cd) { > @@ -165,6 +170,8 @@ static struct arm_smmu_ctx_desc *arm_smmu_alloc_shared_cd(struct mm_struct *mm) > kfree(cd); > out_put_context: > arm64_mm_context_put(mm); > +out_put_mm: > + mmput(mm); > return err < 0 ? ERR_PTR(err) : ret; > } > > @@ -173,6 +180,7 @@ static void arm_smmu_free_shared_cd(struct arm_smmu_ctx_desc *cd) > if (arm_smmu_free_asid(cd)) { > /* Unpin ASID */ > arm64_mm_context_put(cd->mm); > + mmput(cd->mm); > kfree(cd); > } > } > @@ -191,30 +199,6 @@ static void arm_smmu_mm_invalidate_range(struct mmu_notifier *mn, > arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, start, size); > } > > -static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm) > -{ > - struct arm_smmu_mmu_notifier *smmu_mn = mn_to_smmu(mn); > - struct arm_smmu_domain *smmu_domain = smmu_mn->domain; > - > - mutex_lock(&sva_lock); > - if (smmu_mn->cleared) { > - mutex_unlock(&sva_lock); > - return; > - } > - > - /* > - * DMA may still be running. Keep the cd valid to avoid C_BAD_CD events, > - * but disable translation. > - */ > - arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, &quiet_cd); > - > - arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid); > - arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0); > - > - smmu_mn->cleared = true; > - mutex_unlock(&sva_lock); > -} > - > static void arm_smmu_mmu_notifier_free(struct mmu_notifier *mn) > { > kfree(mn_to_smmu(mn)); > @@ -222,7 +206,6 @@ static void arm_smmu_mmu_notifier_free(struct mmu_notifier *mn) > > static const struct mmu_notifier_ops arm_smmu_mmu_notifier_ops = { > .invalidate_range = arm_smmu_mm_invalidate_range, > - .release = arm_smmu_mm_release, > .free_notifier = arm_smmu_mmu_notifier_free, > }; > > @@ -290,14 +273,8 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn) > list_del(&smmu_mn->list); > arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, NULL); > > - /* > - * If we went through clear(), we've already invalidated, and no > - * new TLB entry can have been formed. > - */ > - if (!smmu_mn->cleared) { > - arm_smmu_tlb_inv_asid(smmu_domain->smmu, cd->asid); > - arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0); > - } > + arm_smmu_tlb_inv_asid(smmu_domain->smmu, cd->asid); > + arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0); > > /* Frees smmu_mn */ > mmu_notifier_put(&smmu_mn->mn); > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 627a3ed5ee8f..246670318eda 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -76,12 +76,6 @@ struct arm_smmu_option_prop { > DEFINE_XARRAY_ALLOC1(arm_smmu_asid_xa); > DEFINE_MUTEX(arm_smmu_asid_lock); > > -/* > - * Special value used by SVA when a process dies, to quiesce a CD without > - * disabling it. > - */ > -struct arm_smmu_ctx_desc quiet_cd = { 0 }; > - > static struct arm_smmu_option_prop arm_smmu_options[] = { > { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" }, > { ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"}, > @@ -1047,9 +1041,7 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid, > * (2) Install a secondary CD, for SID+SSID traffic. > * (3) Update ASID of a CD. Atomically write the first 64 bits of the > * CD, then invalidate the old entry and mappings. > - * (4) Quiesce the context without clearing the valid bit. Disable > - * translation, and ignore any translation fault. > - * (5) Remove a secondary CD. > + * (4) Remove a secondary CD. > */ > u64 val; > bool cd_live; > @@ -1065,10 +1057,8 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid, > val = le64_to_cpu(cdptr[0]); > cd_live = !!(val & CTXDESC_CD_0_V); > > - if (!cd) { /* (5) */ > + if (!cd) { /* (4) */ > val = 0; > - } else if (cd == &quiet_cd) { /* (4) */ > - val |= CTXDESC_CD_0_TCR_EPD0; > } else if (cd_live) { /* (3) */ > val &= ~CTXDESC_CD_0_ASID; > val |= FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid); Thanks Jean
Have tested, still got some issue with our openssl-engine.
1. If openssl-engine does not register rsa, nginx works well.
2. If openssl-engine register rsa, nginx also works, but ioasid is not freed when nginx stop.
IMPLEMENT_DYNAMIC_BIND_FN(bind_fn) bind_fn ENGINE_set_RSA(e, rsa_methods())
destroy_fn
If ENGINE_set_RSA is set, nginx start and stop will NOT call destroy_fn. Even rsa_methods is almost new via RSA_meth_new.
In 5.18-rcx, this caused ioasid not freed in nginx start and stop. In 5.17, though destroy_fn is not called, but ioasid is freed when nginx stop, so not noticed this issue before.
Still in checking why ENGINE_set_RSA blocks destroy_fn.
Thanks
| |