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

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

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.

BTW. how about the iommu_detach_device_pasid(), should it also validate
pasid?

> 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)
>   		return -EINVAL;
>   
>   	mutex_lock(&group->mutex);
> +	for_each_group_device(group, device) {
> +		if (pasid >= device->dev->iommu->max_pasids) {
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
> +	}
> +
>   	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
>   	if (curr) {
>   		ret = xa_err(curr) ? : -EBUSY;
> 
> base-commit: 4cece764965020c22cff7665b18a012006359095

-- 
Regards,
Yi Liu