| 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: <f10f54a9-e45f-47f0-8f5e-473daae82665@nvidia.com>
Date: Wed, 7 May 2025 17:21:10 -0700
From: Tushar Dave <tdave@...dia.com>
To: Yi Liu <yi.l.liu@...el.com>, joro@...tes.org, will@...nel.org,
robin.murphy@....com, kevin.tian@...el.com, jgg@...dia.com,
iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Cc: linux-pci@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v3 rc] iommu: Skip PASID validation for devices without
PASID capability
On 5/7/25 06:59, Yi Liu wrote:
>
> On 2025/5/6 05:15, Tushar Dave wrote:
>> Generally PASID support requires ACS settings that usually create
>> single device groups, but there are some niche cases where we can get
>> multi-device groups and still have working PASID support. The primary
>> issue is that PCI switches are not required to treat PASID tagged TLPs
>> specially so appropriate ACS settings are required to route all TLPs to
>> the host bridge if PASID is going to work properly.
>>
>> pci_enable_pasid() does check that each device that will use PASID has
>> the proper ACS settings to achieve this routing.
>>
>> However, no-PASID devices can be combined with PASID capable devices
>> within the same topology using non-uniform ACS settings. In this case
>> the no-PASID devices may not have strict route to host ACS flags and
>> end up being grouped with the PASID devices.
>>
>> This configuration fails to allow use of the PASID within the iommu
>> core code which wrongly checks if the no-PASID device supports PASID.
>>
>> Fix this by ignoring no-PASID devices during the PASID validation. They
>> will never issue a PASID TLP anyhow so they can be ignored.
>>
>> Fixes: c404f55c26fc ("iommu: Validate the PASID in iommu_attach_device_pasid()")
>> Cc: stable@...r.kernel.org
>> Signed-off-by: Tushar Dave <tdave@...dia.com>
>> ---
>>
>> changes in v3:
>> - addressed review comment from Vasant.
>>
>> drivers/iommu/iommu.c | 27 +++++++++++++++++++--------
>> 1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 60aed01e54f2..636fc68a8ec0 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3329,10 +3329,12 @@ static int __iommu_set_group_pasid(struct iommu_domain
>> *domain,
>> int ret;
>> for_each_group_device(group, device) {
>> - ret = domain->ops->set_dev_pasid(domain, device->dev,
>> - pasid, NULL);
>> - if (ret)
>> - goto err_revert;
>> + if (device->dev->iommu->max_pasids > 0) {
>> + ret = domain->ops->set_dev_pasid(domain, device->dev,
>> + pasid, NULL);
>> + if (ret)
>> + goto err_revert;
>> + }
>> }
>> return 0;
>> @@ -3342,7 +3344,8 @@ static int __iommu_set_group_pasid(struct iommu_domain
>> *domain,
>> for_each_group_device(group, device) {
>> if (device == last_gdev)
>> break;
>> - iommu_remove_dev_pasid(device->dev, pasid, domain);
>> + if (device->dev->iommu->max_pasids > 0)
>> + iommu_remove_dev_pasid(device->dev, pasid, domain);
>
> Reviewed-by: Yi Liu <yi.l.liu@...el.com>
>
> with a nit. would it save some loc by adding the max_pasids check in
> iommu_remove_dev_pasid()?
With current code:
drivers/iommu/iommu.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
If I move the pasid check in iommu_remove_dev_pasid(), it would be:
drivers/iommu/iommu.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
e.g.
@@ -3318,8 +3318,9 @@ static void iommu_remove_dev_pasid(struct device *dev,
ioasid_t pasid,
const struct iommu_ops *ops = dev_iommu_ops(dev);
struct iommu_domain *blocked_domain = ops->blocked_domain;
- WARN_ON(blocked_domain->ops->set_dev_pasid(blocked_domain,
- dev, pasid, domain));
+ if (dev->iommu->max_pasids > 0)
+ WARN_ON(blocked_domain->ops->set_dev_pasid(blocked_domain,
+ dev, pasid, domain));
}
static int __iommu_set_group_pasid(struct iommu_domain *domain,
@@ -3329,10 +3330,12 @@ static int __iommu_set_group_pasid(struct iommu_domain
*domain,
int ret;
for_each_group_device(group, device) {
- ret = domain->ops->set_dev_pasid(domain, device->dev,
- pasid, NULL);
- if (ret)
- goto err_revert;
+ if (device->dev->iommu->max_pasids > 0) {
+ ret = domain->ops->set_dev_pasid(domain, device->dev,
+ pasid, NULL);
+ if (ret)
+ goto err_revert;
+ }
}
return 0;
Last hunk remain same as before for iommu_attach_device_pasid()
Let me know.
-Tushar
>
>
>> }
>> return ret;
>> }
>> @@ -3353,8 +3356,10 @@ static void __iommu_remove_group_pasid(struct
>> iommu_group *group,
>> {
>> struct group_device *device;
>> - for_each_group_device(group, device)
>> - iommu_remove_dev_pasid(device->dev, pasid, domain);
>> + for_each_group_device(group, device) {
>> + if (device->dev->iommu->max_pasids > 0)
>> + iommu_remove_dev_pasid(device->dev, pasid, domain);
>> + }
>> }
>> /*
>> @@ -3391,7 +3396,13 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>> mutex_lock(&group->mutex);
>> for_each_group_device(group, device) {
>> - if (pasid >= device->dev->iommu->max_pasids) {
>> + /*
>> + * Skip PASID validation for devices without PASID support
>> + * (max_pasids = 0). These devices cannot issue transactions
>> + * with PASID, so they don't affect group's PASID usage.
>> + */
>> + if ((device->dev->iommu->max_pasids > 0) &&
>> + (pasid >= device->dev->iommu->max_pasids)) {
>> ret = -EINVAL;
>> goto out_unlock;
>> }
>
Powered by blists - more mailing lists