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

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

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():

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