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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Oct 13, 2026 at 02:41:46PM +0530, Aneesh Kumar K.V wrote:
> 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?.

It fails

Probably thats the better answer, AMD should just always fail until
they get their type added.
> >> >> 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?

And the tsm flow must have IOMMUFD_VIOMMU_NO_HWPT

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

This only works if the ops came from a real iommu driver.

Jason