Re: [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Yi Liu <yi.l.liu@intel.com>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Joerg Roedel <jroedel@suse.de>, Kevin Tian <kevin.tian@intel.com>,
	patches@lists.linux.dev, Tony Zhu <tony.zhu@intel.com>,
	Zhangfei Gao <zhangfei.gao@linaro.org>
Subject: Re: [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid()
Date: Wed, 27 Mar 2024 11:27:59 -0300	[thread overview]
Message-ID: <20240327142759.GH946323@nvidia.com> (raw)
In-Reply-To: <e8e0f7cb-b4a7-433f-a204-73dc0a976246@intel.com>

On Wed, Mar 27, 2024 at 10:14:45PM +0800, Yi Liu wrote:
> On 2024/3/27 21:41, Jason Gunthorpe wrote:
> > The SVA code checks that the PASID is valid for the device when assigning
> > the PASID to the MM, but the normal PAGING related path does not check it.
> > > Devices that don't support PASID or PASID values too large for the device
> > should not invoke the driver callback. The drivers should rely on the
> > core code for this enforcement.
> 
> I agree it is reasonable to enforce it in the core. But I'm not sure if a
> fix tag is needed or not. As far as I know, intel iommu driver supports
> attaching both the SVA and DMA type (PAGING) domain to pasid. Intel iommu
> driver checks the max pasid in intel_pasid_get_entry() of
> drivers/iommu/intel/pasid.c.
> I'm not sure about ARM and AMD side, if the two drivers only support SVA
> domain, and have the max pasid check. Then fix tag may be not necessary as
> all the related paths are in good shape on the max pasid check before this
> fix. :)

Ah, I could not find the max pasid check in the Intel driver.

> > Fixes: 16603704559c7a68 ("iommu: Add attach/detach_dev_pasid iommu interfaces")
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   drivers/iommu/iommu.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 098869007c69e5..a95a483def2d2a 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -3354,6 +3354,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> >   {
> >   	/* Caller must be a probed driver on dev */
> >   	struct iommu_group *group = dev->iommu_group;
> > +	struct group_device *device;
> >   	void *curr;
> >   	int ret;
> > @@ -3363,10 +3364,18 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> >   	if (!group)
> >   		return -ENODEV;
> > -	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
> > +	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
> > +	    pasid == IOMMU_NO_PASID)
> 
> perhaps this can be a separate patch as it means this API does not support
> NO_PASID attachment.

It never did? For something like Intel you can't use this API to
change the RID's domain, it would break things. It is all the same
topic - missing PASID validation.

That alone is worth the fixes :)

Thanks,
Jason