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

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

sashiko-bot@kernel.org writes:

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

No vIOMMU type has been added for any IOMMU other than Arm. The
documentation also considers IOMMU_VIOMMU_TYPE_DEFAULT reserved.

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

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

	ret = arm_smmu_validate_parent(smmu, parent_domain);
	if (ret)
		return ret;



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

-aneesh