Re: [PATCH v5 1/6] iommu/msm: Fix error-out routine in msm_iommu_attach_dev()

Jason Gunthorpe
2022-09-22T17:23:56Z

On Thu, Sep 22, 2022 at 01:53:44AM -0700, Nicolin Chen wrote:

> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index 6a24aa804ea3..14df722f0060 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -394,6 +394,34 @@ static struct iommu_device *msm_iommu_probe_device(struct device *dev)
> return &iommu->iommu;
> }
>
> +static void msm_iommu_detach_dev(struct iommu_domain *domain,
> + struct device *dev)
> +{
> + struct msm_priv *priv = to_msm_priv(domain);
> + unsigned long flags;
> + struct msm_iommu_dev *iommu;
> + struct msm_iommu_ctx_dev *master;
> + int ret;
> +
> + free_io_pgtable_ops(priv->iop);
> +
> + spin_lock_irqsave(&msm_iommu_lock, flags);
> + list_for_each_entry(iommu, &priv->list_attached, dom_node) {
> + ret = __enable_clocks(iommu);
> + if (ret)
> + goto fail;
> +
> + list_for_each_entry(master, &iommu->ctx_list, list) {
> + msm_iommu_free_ctx(iommu->context_map, master->num);
> + __reset_context(iommu->base, master->num);

This isn't safe if the loop in msm_iommu_attach_dev() exits
early.

Firstly the it leaves master->num set to -ERRNO so clear_bit will
corrupt memory

Secondly if the attach loop gets half way through the remaining
entries have master->num == 0 and so the clear_bit() will corrupt an
otherwise valid entry.

Fixing all of this properly looks like a big deal, I think we should
drop this patch from the series.

Jason