linux-kernel - Re: [PATCH -next] iommu/dma: Fix missing mutex_init() in iommu_get_msi

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, 1 Aug 2022 20:48:33 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Yang Yingliang <yangyingliang@...wei.com>,
        Joerg Roedel <joro@...tes.org>
Cc:     linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
        iommu@...ts.linux-foundation.org, will@...nel.org,
        yf.wang@...iatek.com
Subject: Re: [PATCH -next] iommu/dma: Fix missing mutex_init() in
 iommu_get_msi_cookie()

On 2022-07-18 07:42, Yang Yingliang wrote:
> Hi,
> 
> On 2022/7/15 17:28, Robin Murphy wrote:
>> On 2022-07-15 08:49, Joerg Roedel wrote:
>>> Adding Robin.
>>>
>>> On Mon, Jun 27, 2022 at 04:55:33PM +0800, Yang Yingliang wrote:
>>>> cookie_alloc() is called by iommu_get_dma_cookie() and 
>>>> iommu_get_msi_cookie(),
>>>> but the mutex is only initialized in iommu_get_dma_cookie(), move 
>>>> mutex_init()
>>>> into cookie_alloc() to make sure the mutex will be initialized.
>>
>> The mutex is only used in iommu_dma_init_domain(), which is only 
>> called by iommu_setup_dma_ops() for IOMMU_DOMAIN_DMA domains. How is 
>> there a problem here?
> It's no problem now, but I thinks it's better to initialize the 'mutex' 
> in cookie_alloc() to make code stronger.

Stronger against what, though? The sole reason this mutex exists at all 
is as a temporary measure to protect the IOVA domain from concurrent 
initialisation - I suppose in hindsight it might have made sense to 
define it inside the union, but I don't see much point in churning that 
now. I'd rather spend the time on continuing to get the 
iommu_probe_device() path sorted out so that we don't have the whole 
problematic driver-probe-time-replay mess in the first place and this 
mutex can be reverted ASAP.

>>>> Fixes: ac9a5d522bb8 ("iommu/dma: Fix race condition during 
>>>> iova_domain initialization")
>>>> Reported-by: Hulk Robot <hulkci@...wei.com>

Please teach your robot to care about things that actually matter. All 
it's "reporting" here is that it isn't clever enough to follow control 
flow across multiple compilation units, otherwise it should have seen 
the matching iommu_is_dma_domain() checks between __iommu_domain_alloc() 
and iommu_setup_dma_ops(). Having to explain this is not a good use of 
the kernel community's time and effort.

Thanks,
Robin.

>>>> Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
>>>> ---
>>>>   drivers/iommu/dma-iommu.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>> index 1910f4f1612b..e29157380c48 100644
>>>> --- a/drivers/iommu/dma-iommu.c
>>>> +++ b/drivers/iommu/dma-iommu.c
>>>> @@ -294,6 +294,7 @@ static struct iommu_dma_cookie 
>>>> *cookie_alloc(enum iommu_dma_cookie_type type)
>>>>       if (cookie) {
>>>>           INIT_LIST_HEAD(&cookie->msi_page_list);
>>>>           cookie->type = type;
>>>> +        mutex_init(&cookie->mutex);
>>>>       }
>>>>       return cookie;
>>>>   }
>>>> @@ -311,7 +312,6 @@ int iommu_get_dma_cookie(struct iommu_domain 
>>>> *domain)
>>>>       if (!domain->iova_cookie)
>>>>           return -ENOMEM;
>>>>   -    mutex_init(&domain->iova_cookie->mutex);
>>>>       return 0;
>>>>   }
>>>>   --
>>>> 2.25.1
>> .