Re: [PATCH v7 13/16] iommufd/viommu: Select vIOMMU operations before allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] iommu: type confusion in iommufd_viommu_alloc_ioctl() parent domain
- [High] iommu: dead code because amd_iommu_alloc_domain_nested is not assigned
- [Low] iommu: missing viommu_type validation in amd_iommufd_get_viommu_ops
--

commit ea74b8b169fc38edaee15fa883c362684ddec72d
Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
Subject: iommufd/viommu: Select vIOMMU operations before allocation

This commit introduces a new get_viommu_ops callback to select vIOMMU
operations based on the device and requested type before allocation. It
moves the size and initialization callbacks into iommufd_viommu_ops and
converts multiple physical IOMMU drivers to use the new interface.

> diff --git a/drivers/iommu/amd/iommufd.c b/drivers/iommu/amd/iommufd.c
> index 52300b867c1f2..c1e132382adc7 100644
> --- a/drivers/iommu/amd/iommufd.c
> +++ b/drivers/iommu/amd/iommufd.c
> @@ -32,13 +32,21 @@ void *amd_iommufd_hw_info(struct device *dev, u32 *length, enum iommu_hw_info_ty
>  	return hwinfo;
>  }
>  
> -size_t amd_iommufd_get_viommu_size(struct device *dev, enum iommu_viommu_type viommu_type)
> +static size_t amd_iommufd_get_viommu_size(struct device *dev,
> +		enum iommu_viommu_type viommu_type)
>  {
>  	return VIOMMU_STRUCT_SIZE(struct amd_iommu_viommu, core);
>  }
>  
> -int amd_iommufd_viommu_init(struct iommufd_viommu *viommu, struct iommu_domain *parent,
> -			    const struct iommu_user_data *user_data)
> +const struct iommufd_viommu_ops *
> +amd_iommufd_get_viommu_ops(struct device *dev, enum iommu_viommu_type viommu_type)
> +{
> +	return &amd_viommu_ops;
> +}

[Severity: Low]
Does this function need to validate the requested viommu_type?

Currently, amd_iommufd_get_viommu_ops() unconditionally returns the ops
structure without checking the type argument. If a user requests an arbitrary
type like IOMMU_VIOMMU_TYPE_ARM_SMMUV3 on an AMD device, the core iommufd
framework might track the vIOMMU under a mismatched architectural type
identifier.

The ARM and selftest backends appear to validate this argument against their
supported hardware types before returning the operations.

[ ... ]

> @@ -73,5 +79,7 @@ static void amd_iommufd_viommu_destroy(struct iommufd_viommu *viommu)
>   * struct iommufd_viommu_ops - vIOMMU specific operations
>   */
>  static const struct iommufd_viommu_ops amd_viommu_ops = {
> +	.get_viommu_size = amd_iommufd_get_viommu_size,
> +	.viommu_init = amd_iommufd_viommu_init,
>  	.destroy = amd_iommufd_viommu_destroy,
>  };

[Severity: High]
Is the alloc_domain_nested callback missing from this operations structure?

The commit message and comments in amd_iommu_alloc_domain_nested() indicate
it should be assigned to the viommu operations. However, it is omitted here.

If a user attempts to allocate an AMD nested domain using IOMMU_HWPT_ALLOC,
won't the core framework reject it with -EOPNOTSUPP because
viommu->ops->alloc_domain_nested is NULL?

> diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
> index f3d5b5a7eb4a4..e95138ae71d5b 100644
> --- a/drivers/iommu/iommufd/viommu.c
> +++ b/drivers/iommu/iommufd/viommu.c
[ ... ]
> @@ -103,16 +105,12 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
>  	 */
>  	viommu->iommu_dev = iommu_dev;
>  
> -	rc = ops->viommu_init(viommu, hwpt_paging->common.domain,
> +	rc = ops->viommu_init(viommu, idev->dev,
> +			      hwpt_paging->common.domain,
>  			      user_data.len ? &user_data : NULL);

[Severity: Critical]
Is there a missing check to ensure the domain is owned by the same driver
before passing it to viommu_init?

In iommufd_viommu_alloc_ioctl(), hwpt_paging->common.domain is passed directly
to the device driver's viommu_init callback. Unlike in
iommufd_hw_pagetable_alloc() which checks if parent->common.domain->owner
matches the ops, this check appears to be omitted here.

If a user allocates a paging HWPT for a mock device, and then calls
IOMMU_VIOMMU_ALLOC passing an AMD device ID but the mock HWPT ID, could the
receiving driver blindly cast the mismatched domain? For instance, AMD's
viommu_init uses to_pdomain() on the parent domain. This looks like it could
lead to type confusion and out-of-bounds accesses on the domain structure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008055955.4014342-1-aneesh.kumar@kernel.org?part=13