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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Jason Gunthorpe <jgg@nvidia.com> writes:

> On Mon, Oct 12, 2026 at 03:07:57PM +0530, Aneesh Kumar K.V wrote:
>> > 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.
>> >
>> 
>> No vIOMMU type has been added for any IOMMU other than Arm. The
>> documentation also considers IOMMU_VIOMMU_TYPE_DEFAULT reserved.
>
> This is a bug in the AMD driver we need to fix, send a patch? It
> should only accept DEFAULT, not every type.
>

But the comment says "Reserved"

 * @IOMMU_VIOMMU_TYPE_DEFAULT: Reserved for future use

What is unclear is what using that type on Arm means. Does it imply
IOMMU_VIOMMU_TYPE_ARM_SMMUV3?.

>
>> >> @@ -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?
>> >
>> 
>> It is unclear whether nested domain support is fully integrated for AMD.
>> I see amd_iommu_alloc_domain_nested(), but cannot find any callers.
>
> There is a series on the list to complete more of this, it is fine the
> way you have it.
>
>
>> >> 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?
>> 
>> The viommu_init callback needs to perform this check. For example, the
>> Arm implementation does:
>
> No, the check is missing. If a domain is being used then the core code
> needs to check it, not the driver.
>
> When the ops is first obtained it should be checked along with the
> domain to be sure it is compatible.
>
> The NULL parent domain on the TSM path should be enforced in the core
> code since there is nothing to check there.
>

So, if IOMMUFD_VIOMMU_NO_HWPT is not set, the HWPT must have been
created with IOMMU_HWPT_ALLOC_NEST_PARENT, and its domain owner must
match iommu_dev->ops?

Something like:

	idev = iommufd_get_device(ucmd, cmd->dev_id);
	iommu_dev = iommufd_device_get_iommu_dev(idev);

	if (ops->flags & IOMMUFD_VIOMMU_NO_HWPT) {
		if (cmd->hwpt_id) {
			rc = -EINVAL;
			goto out_put_idev;
		}
	} else {
		hwpt_paging = iommufd_get_hwpt_paging(ucmd, cmd->hwpt_id);
		if (IS_ERR(hwpt_paging)) {
			rc = PTR_ERR(hwpt_paging);
			goto out_put_idev;
		}
		if (!hwpt_paging->nest_parent || !iommu_dev ||
		    hwpt_paging->common.domain->owner != iommu_dev->ops) {
			rc = -EINVAL;
			goto out_put_hwpt;
		}
	}

-aneesh