| 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
| ||
|
Message-ID: <9a88b12e-7169-7c7f-09a1-eef86f75891c@arm.com>
Date: Thu, 9 Mar 2023 11:23:00 +0000
From: Robin Murphy <robin.murphy@....com>
To: Baolu Lu <baolu.lu@...ux.intel.com>,
Chunyan Zhang <chunyan.zhang@...soc.com>,
Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>
Cc: 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: release dma buffer to avoid memory leak
On 2023-03-09 02:50, Baolu Lu wrote:
> On 3/8/23 8:37 PM, Robin Murphy wrote:
>> On 2023-03-08 03:41, Chunyan Zhang wrote:
>>> Release page table DMA buffer when the IOMMU domain is not used:
>>>
>>> - Domain freed.
>>>
>>> - IOMMU is attaching to a new domain.
>>> Since one sprd IOMMU servers only one client device, if the IOMMU has
>>> been attached to other domain, it has to be detached first, that's
>>> saying the DMA buffer should be released, otherwise that would
>>> cause memory leak issue.
>>
>> This is clearly wrong; domain resources should only be freed when the
>> domain is freed.
>
> Agreed. Perhaps, in the attach path:
>
> if (!dom->pgt_va)
> dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size,
> &dom->pgt_pa, GFP_KERNEL);
> ?
Why? dom->pgt_va will always be NULL if dom->sdev is NULL, and once
dom->sdev is set then that allocation is not reachable (of course this
means the driver also has a separate bug where reattaching to a
previously-used domain will erroneously fail, but that's not a memory
leak as such). The only thing to recognise here is that the allocation
is logically not part of the .attach_dev operation, but really a one-off
deferred part of .domain_alloc, and thus it should be balanced in .free,
not anywhere else.
Thanks,
Robin.
>
> Best regards,
> baolu
>
>> Just because a caller has detached from a domain doesn't mean that
>> they can't reattach to it later and expect the previous mappings to
>> still be in place - it has nothing to do with how many devices or
>> domains can be active at once.
>>
>> Thanks,
>> Robin.
>>
>>> Signed-off-by: Chunyan Zhang <chunyan.zhang@...soc.com>
>>> ---
>>> V2:
>>> * Added some comment in sprd_iommu_attach_device() for the reason
>>> of calling sprd_iommu_cleanup().
>>>
>>> V1: https://lkml.org/lkml/2023/2/10/198
>>> ---
>>> drivers/iommu/sprd-iommu.c | 46 ++++++++++++++++++++++++++++++++------
>>> 1 file changed, 39 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
>>> index ae94d74b73f4..fb2f96df3bca 100644
>>> --- a/drivers/iommu/sprd-iommu.c
>>> +++ b/drivers/iommu/sprd-iommu.c
>>> @@ -62,6 +62,7 @@ enum sprd_iommu_version {
>>> * @eb: gate clock which controls IOMMU access
>>> */
>>> struct sprd_iommu_device {
>>> + struct sprd_iommu_domain *dom;
>>> enum sprd_iommu_version ver;
>>> u32 *prot_page_va;
>>> dma_addr_t prot_page_pa;
>>> @@ -151,13 +152,6 @@ static struct iommu_domain
>>> *sprd_iommu_domain_alloc(unsigned int domain_type)
>>> return &dom->domain;
>>> }
>>> -static void sprd_iommu_domain_free(struct iommu_domain *domain)
>>> -{
>>> - struct sprd_iommu_domain *dom = to_sprd_domain(domain);
>>> -
>>> - kfree(dom);
>>> -}
>>> -
>>> static void sprd_iommu_first_vpn(struct sprd_iommu_domain *dom)
>>> {
>>> struct sprd_iommu_device *sdev = dom->sdev;
>>> @@ -230,6 +224,29 @@ static void sprd_iommu_hw_en(struct
>>> sprd_iommu_device *sdev, bool en)
>>> sprd_iommu_update_bits(sdev, reg_cfg, mask, 0, val);
>>> }
>>> +static void sprd_iommu_cleanup(struct sprd_iommu_device *sdev)
>>> +{
>>> + struct sprd_iommu_domain *dom = sdev->dom;
>>> + size_t pgt_size = sprd_iommu_pgt_size(&dom->domain);
>>> +
>>> + dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
>>> + dom->sdev = NULL;
>>> + sdev->dom = NULL;
>>> + sprd_iommu_hw_en(sdev, false);
>>> +}
>>> +
>>> +static void sprd_iommu_domain_free(struct iommu_domain *domain)
>>> +{
>>> + struct sprd_iommu_domain *dom = to_sprd_domain(domain);
>>> + struct sprd_iommu_device *sdev = dom->sdev;
>>> +
>>> + /* Free DMA buffer first if the domain has been attached */
>>> + if (sdev)
>>> + sprd_iommu_cleanup(sdev);
>>> +
>>> + kfree(dom);
>>> +}
>>> +
>>> static int sprd_iommu_attach_device(struct iommu_domain *domain,
>>> struct device *dev)
>>> {
>>> @@ -237,14 +254,29 @@ static int sprd_iommu_attach_device(struct
>>> iommu_domain *domain,
>>> struct sprd_iommu_domain *dom = to_sprd_domain(domain);
>>> size_t pgt_size = sprd_iommu_pgt_size(domain);
>>> + /* Return directly if the domain attached to IOMMU already */
>>> if (dom->sdev)
>>> return -EINVAL;
>>> + /* The IOMMU already attached to a domain */
>>> + if (sdev->dom) {
>>> + if (sdev->dom == dom)
>>> + return 0;
>>> +
>>> + /*
>>> + * Clean up the previous domain, one sprd IOMMU servers only
>>> + * one client device, if the IOMMU has been attached to other
>>> + * domain, it has to be detached first.
>>> + */
>>> + sprd_iommu_cleanup(sdev);
>>> + }
>>> +
>>> dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size,
>>> &dom->pgt_pa, GFP_KERNEL);
>>> if (!dom->pgt_va)
>>> return -ENOMEM;
>>> dom->sdev = sdev;
>>> + sdev->dom = dom;
>>> sprd_iommu_first_ppn(dom);
>>> sprd_iommu_first_vpn(dom);
>>
Powered by blists - more mailing lists