linux-kernel - Re: [PATCH v5 1/6] iommu/msm: Fix error-out routine in msm_iommu_attach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yyyo1RvzviIJLPMb@Asurada-Nvidia>
Date:   Thu, 22 Sep 2022 11:26:29 -0700
From:   Nicolin Chen <nicolinc@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     <agross@...nel.org>, <bjorn.andersson@...aro.org>,
        <konrad.dybcio@...ainline.org>, <joro@...tes.org>,
        <will@...nel.org>, <robin.murphy@....com>,
        <sricharan@...eaurora.org>, <kevin.tian@...el.com>,
        <linux-arm-msm@...r.kernel.org>, <iommu@...ts.linux.dev>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 1/6] iommu/msm: Fix error-out routine in
 msm_iommu_attach_dev()

On Thu, Sep 22, 2022 at 02:23:22PM -0300, Jason Gunthorpe wrote:

> > +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.

OK. Let me resend a version dropping this one. Thanks!