linux-kernel - Re: [PATCH 1/4] iommu/vt-d: Implement set device pasid op for default domain

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]

Date:   Fri, 3 Mar 2023 12:38:21 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
        Jason Gunthorpe <jgg@...dia.com>,
        Joerg Roedel <joro@...tes.org>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        "vkoul@...nel.org" <vkoul@...nel.org>
Cc:     baolu.lu@...ux.intel.com, Robin Murphy <robin.murphy@....com>,
        Will Deacon <will@...nel.org>,
        David Woodhouse <dwmw2@...radead.org>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "Liu, Yi L" <yi.l.liu@...el.com>,
        "Yu, Fenghua" <fenghua.yu@...el.com>,
        "Jiang, Dave" <dave.jiang@...el.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        "Zanussi, Tom" <tom.zanussi@...el.com>
Subject: Re: [PATCH 1/4] iommu/vt-d: Implement set device pasid op for default
 domain

On 3/3/23 11:02 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@...ux.intel.com>
>> Sent: Friday, March 3, 2023 10:49 AM
>>
>> On 3/3/23 10:36 AM, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@...ux.intel.com>
>>>> Sent: Thursday, March 2, 2023 10:07 PM
>>>>> +
>>>>> +	if (!sm_supported(iommu) || !info)
>>>>
>>>> @info has been referenced. !info check makes no sense.
>>>>
>>>> Add pasid_supported(iommu).
>>>>
>>>> Do you need to check whether the domain is compatible for this rid
>>>> pasid?
>>>
>>> what kind of compatibility is concerned here? In concept a pasid
>>> can be attached to any domain if it has been successfully attached
>>> to rid. Probably we can add a check here that RID2PASID must
>>> point to the domain already.
>>
>> "...if it has been successfully attached to rid..."
>>
>> We should not have this assumption in iommu driver's callback. The iommu
>> driver has no (and should not have) knowledge about the history of any
>> domain.
> 
> but this is an op for default domain which must have been attached
> to RID2PASID and any compatibility check between this domain and device
> should be passed.

This is an op for DMA, DMA-FQ and UNMANAGED domain. The IOMMU driver
doesn't need to interpret the default domain concept. :-)

> 
> We can have another set_pasid for unmanaged which then need similar
> check as prepare_domain_attach_device() does.

 From the perspective of the iommu driver, there's no essential
difference between DMA and UNMANAGED domains. So almost all IOMMU
drivers maintain a single set of domain ops for them.

>>
>>>
>>>>
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	if (WARN_ON(pasid == PASID_RID2PASID))
>>>>> +		return -EINVAL;
>>>>
>>>> Add a call to domain_attach_iommu() here to get a refcount of the
>> domain
>>>> ID. And call domain_detach_iommu() in
>> intel_iommu_remove_dev_pasid().
>>>>
>>>
>>> Is it necessary? iommu core doesn't allow taking ownership
>>> if !xa_empty(&group->pasid_array) so if this pasid attach succeeds
>>> this device cannot be attached to another domain before pasid
>>> detach is done on the current domain.
>>
>> It's not about the pasid, but the domain id.
>>
>> This domain's id will be set to a field of the device's pasid entry. It
>> must get a refcount of that domain id to avoid use after free.
>>
> 
> If the domain still has attached device (due to this pasid usage) how could
> domain id be freed?

The Intel IOMMU driver uses a user counter to determine when the domain
id could be freed.

Best regards,
baolu