Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit

zhangfei.gao
2022-04-21T06:51:39Z

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