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

[Yi Liu <yi.l.liu@intel.com>]

On 2024/3/27 22:27, Jason Gunthorpe wrote:
> 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.

I see. May have a look at the below code. When get pasid entry, it would
check the pasid against the return value of intel_pasid_get_dev_max_id(dev)

https://github.com/torvalds/linux/blob/7033999ecd7b8cf9ea59265035a0150961e023ee/drivers/iommu/intel/pasid.c#L137

> 
>>> 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.

aha, yes. one patch is ok to me as well. :)

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

> That alone is worth the fixes :)

-- 
Regards,
Yi Liu