linux-kernel - Re: [PATCH V2] iommu: sprd: fix a dma buffer leak issue

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] [day] [month] [year] [list]

Date:   Mon, 6 Mar 2023 17:29:23 +0800
From:   Chunyan Zhang <zhang.lyra@...il.com>
To:     Baolu Lu <baolu.lu@...ux.intel.com>
Cc:     Chunyan Zhang <chunyan.zhang@...soc.com>,
        Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>, iommu@...ts.linux.dev,
        Baolin Wang <baolin.wang@...ux.alibaba.com>,
        Orson Zhai <orsonzhai@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2] iommu: sprd: fix a dma buffer leak issue

On Mon, 6 Mar 2023 at 13:57, Baolu Lu <baolu.lu@...ux.intel.com> wrote:
>
> On 2023/3/6 13:26, Chunyan Zhang wrote:
> > The DMA buffer used to store the address mpping table is alloced when
> > attaching device to a domain, and had been released in .detach_dev()
> > callback which will never be called since this driver supports default
> > domain, that will cause memory leak.
> >
> > Move the dma buffer free before freeing sprd iommu domain to fix
> > this issue.
> >
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@...soc.com>
> > ---
> > V2: Modified commit message
> > ---
> >   drivers/iommu/sprd-iommu.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> > index ae94d74b73f4..4de2e79d2226 100644
> > --- a/drivers/iommu/sprd-iommu.c
> > +++ b/drivers/iommu/sprd-iommu.c
> > @@ -154,6 +154,17 @@ static struct iommu_domain *sprd_iommu_domain_alloc(unsigned int domain_type)
> >   static void sprd_iommu_domain_free(struct iommu_domain *domain)
> >   {
> >       struct sprd_iommu_domain *dom = to_sprd_domain(domain);
> > +     size_t pgt_size = sprd_iommu_pgt_size(domain);
> > +     struct sprd_iommu_device *sdev = dom->sdev;
> > +
> > +     /*
> > +      * If the domain has been attached to a device already,
> > +      * free the dma buffer first.
>
> Is it possible that a domain is attached to multiple devices or attached
> to a device multiple times? If so, perhaps the memory is also leaked in
> sprd_iommu_attach_device():

I managed the wrong patch, sorry for the big mistake.

>
> 233 static int sprd_iommu_attach_device(struct iommu_domain *domain,
> 234                                     struct device *dev)
> 235 {
> 236         struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
> 237         struct sprd_iommu_domain *dom = to_sprd_domain(domain);
> 238         size_t pgt_size = sprd_iommu_pgt_size(domain);
> 239
> 240         if (dom->sdev)
> 241                 return -EINVAL;
> 242
> 243         dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size,
> &dom->pgt_pa, GFP_KERNEL);
> 244         if (!dom->pgt_va)
> 245                 return -ENOMEM;
>
> as dom->pgt_va is not checked before setting a new pointer.
>
> If sprd iommu driver only supports one-time use of domain， perhaps put

Yes, it is the case for sprd iommu. I will add a comment in the next version.

Many thanks for the review,
Chunyan

> a comment around above code so that people can take care of it when the
> assumption changes.
>
> > +      */
> > +     if (sdev) {
> > +             dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
> > +             dom->sdev = NULL;
> > +     }
> >
> >       kfree(dom);
> >   }
>
> Best regards,
> baolu