linux-kernel - Re: [PATCH RFC v2 3/4] iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset

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: <20250630123814.GS167785@nvidia.com>
Date: Mon, 30 Jun 2025 09:38:14 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>
Cc: Nicolin Chen <nicolinc@...dia.com>, joro@...tes.org, will@...nel.org,
	robin.murphy@....com, rafael@...nel.org, lenb@...nel.org,
	bhelgaas@...gle.com, iommu@...ts.linux.dev,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
	linux-pci@...r.kernel.org, patches@...ts.linux.dev,
	pjaroszynski@...dia.com, vsethi@...dia.com, helgaas@...nel.org
Subject: Re: [PATCH RFC v2 3/4] iommu: Introduce iommu_dev_reset_prepare()
 and iommu_dev_reset_done()

On Sat, Jun 28, 2025 at 09:28:12PM +0800, Baolu Lu wrote:
 
> Does this mean the IOMMU driver should disable ATS when ops-
> >blocked_domain is used? This might not be feasible because ops-
> >blocked_domain might possibly be attached to a PASID of a device,
> while other PASIDs still use ATS for functionality.

No.. The above should be setting everything, including PASIDs to the
blocked domain.

The driver doesn't have to disable ATS at the device, but ARM does.

It does have to stop issuing invalidations, which is part of the
definition of blocked in the first place.

> >   - This only works for IOMMU drivers that will not issue ATS invalidation
> >     requests to the device, after it's docked at ops->blocked_domain.
> > Driver should fix itself to align with the aforementioned notes.
> 
> My understanding of the requirements for the iommu drivers is: when all
> PASIDs are docked in the blocking DMA state, the IOMMU driver should:
> 
> - Flush all outstanding ATS invalidation requests;

Arugably driver needs to have serialized ATS invalidation
synchronously during the change to the blocked domain. The prior
paging domain could be immediately freed so lingering invalidations
are probably an existing bug.

> - Stop issuing any further ATS invalidations;

Yes

> - Configure the hardware to reject further ATS translation requests.

Not required. Blocked domain inherently responds to all ATS
translation requests with no-present which is not allowed to be
cached.

> > +int iommu_dev_reset_prepare(struct device *dev)
> > +{
> > +	const struct iommu_ops *ops;
> > +	struct iommu_group *group;
> > +	unsigned long pasid;
> > +	void *entry;
> > +	int ret = 0;
> > +
> > +	if (!dev_has_iommu(dev))
> > +		return 0;
> > +
> > +	if (dev->iommu->require_direct) {
> > +		dev_warn(
> > +			dev,
> > +			"Firmware has requested this device have a 1:1 IOMMU mapping, rejecting configuring the device without a 1:1 mapping. Contact your platform vendor.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* group will be put in iommu_dev_reset_done() */
> > +	group = iommu_group_get(dev);
> > +
> > +	/* Caller ensures no racy iommu_release_device(), so this won't UAF */
> > +	mutex_lock(&group->mutex);
> > +
> > +	ops = dev_iommu_ops(dev);
> > +	if (!ops->blocked_domain) {
> > +		dev_warn(dev,
> > +			 "IOMMU driver doesn't support IOMMU_DOMAIN_BLOCKED\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto unlock;
> > +	}
> > +
> > +	device_to_group_device(dev)->pending_reset = true;
> > +
> > +	/* Device is already attached to the blocked_domain. Nothing to do */
> > +	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
> > +		goto unlock;
> 
> "group->domain->type == IOMMU_DOMAIN_BLOCKED" means that IOMMU_NO_PASID
> is docked in the blocking DMA state, but it doesn't imply that other
> PASIDs are also in the blocking DMA state. Therefore, we might still
> need the following lines to handle other PASIDs.

Yes, we always have to check the xarray.

> On the other hand, perhaps we should use "group->domain == ops-
> >blocked_domain" instead of "group->domain->type ==
> IOMMU_DOMAIN_BLOCKED" to make the code consistent with the commit
> message.

ops->blocked_domain is not good, we support devices without static
blocking domain. But yes, using DOMAIN_BLOCKED is not greap, there is
a group->blocked_domain that should be used and will dynamicaly create
an empty paging domain if needed.

Jason