Re: [PATCH v5 09/15] iommu/vt-d: Check ownership for PASIDs from user-space

[Auger Eric]

Yi,

On 7/20/20 12:18 PM, Liu, Yi L wrote:
> Hi Eric,
> 
>> From: Auger Eric <eric.auger@xxxxxxxxxx>
>> Sent: Monday, July 20, 2020 12:06 AM
>>
>> Hi Yi,
>>
>> On 7/12/20 1:21 PM, Liu Yi L wrote:
>>> When an IOMMU domain with nesting attribute is used for guest SVA, a
>>> system-wide PASID is allocated for binding with the device and the domain.
>>> For security reason, we need to check the PASID passsed from user-space.
>> passed
> 
> got it.
> 
>>> e.g. page table bind/unbind and PASID related cache invalidation.
>>>
>>> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
>>> CC: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
>>> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
>>> Cc: Eric Auger <eric.auger@xxxxxxxxxx>
>>> Cc: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
>>> Cc: Joerg Roedel <joro@xxxxxxxxxx>
>>> Cc: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
>>> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
>>> ---
>>>  drivers/iommu/intel/iommu.c | 10 ++++++++++
>>>  drivers/iommu/intel/svm.c   |  7 +++++--
>>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index 4d54198..a9504cb 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -5436,6 +5436,7 @@ intel_iommu_sva_invalidate(struct iommu_domain
>> *domain, struct device *dev,
>>>  		int granu = 0;
>>>  		u64 pasid = 0;
>>>  		u64 addr = 0;
>>> +		void *pdata;
>>>
>>>  		granu = to_vtd_granularity(cache_type, inv_info->granularity);
>>>  		if (granu == -EINVAL) {
>>> @@ -5456,6 +5457,15 @@ intel_iommu_sva_invalidate(struct iommu_domain
>> *domain, struct device *dev,
>>>  			 (inv_info->granu.addr_info.flags &
>> IOMMU_INV_ADDR_FLAGS_PASID))
>>>  			pasid = inv_info->granu.addr_info.pasid;
>>>
>>> +		pdata = ioasid_find(dmar_domain->ioasid_sid, pasid, NULL);
>>> +		if (!pdata) {
>>> +			ret = -EINVAL;
>>> +			goto out_unlock;
>>> +		} else if (IS_ERR(pdata)) {
>>> +			ret = PTR_ERR(pdata);
>>> +			goto out_unlock;
>>> +		}
>>> +
>>>  		switch (BIT(cache_type)) {
>>>  		case IOMMU_CACHE_INV_TYPE_IOTLB:
>>>  			/* HW will ignore LSB bits based on address mask */
>>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>>> index d2c0e1a..212dee0 100644
>>> --- a/drivers/iommu/intel/svm.c
>>> +++ b/drivers/iommu/intel/svm.c
>>> @@ -319,7 +319,7 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain,
>> struct device *dev,
>>>  	dmar_domain = to_dmar_domain(domain);
>>>
>>>  	mutex_lock(&pasid_mutex);
>>> -	svm = ioasid_find(INVALID_IOASID_SET, data->hpasid, NULL);
I meant while using INVALID_IOASID_SET instead of the actual
dmar_domain->ioasid_sid. But I think I've now recovered, the asset is
simply not used ;-)
>> I do not get what the call was supposed to do before that patch?
> 
> you mean patch 10/15 by "that patch", right? the ownership check should
> be done as to prevent illegal bind request from userspace. before patch
> 10/15, it should be added.
> 
>>> +	svm = ioasid_find(dmar_domain->ioasid_sid, data->hpasid, NULL);
>>>  	if (IS_ERR(svm)) {
>>>  		ret = PTR_ERR(svm);
>>>  		goto out;
>>> @@ -436,6 +436,7 @@ int intel_svm_unbind_gpasid(struct iommu_domain
>> *domain,
>>>  			    struct device *dev, ioasid_t pasid)
>>>  {
>>>  	struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
>>> +	struct dmar_domain *dmar_domain;
>>>  	struct intel_svm_dev *sdev;
>>>  	struct intel_svm *svm;
>>>  	int ret = -EINVAL;
>>> @@ -443,8 +444,10 @@ int intel_svm_unbind_gpasid(struct iommu_domain
>> *domain,
>>>  	if (WARN_ON(!iommu))
>>>  		return -EINVAL;
>>>
>>> +	dmar_domain = to_dmar_domain(domain);
>>> +
>>>  	mutex_lock(&pasid_mutex);
>>> -	svm = ioasid_find(INVALID_IOASID_SET, pasid, NULL);
>>> +	svm = ioasid_find(dmar_domain->ioasid_sid, pasid, NULL);
>> just to make sure, about the locking, can't domain->ioasid_sid change
>> under the hood?
> 
> I guess not. intel_svm_unbind_gpasid() and iommu_domain_set_attr()
> is called by vfio today, and within vfio, there is vfio_iommu->lock.
OK

Thanks

Eric
> 
> Regards,
> Yi Liu
> 
>>
>> Thanks
>>
>> Eric
>>>  	if (!svm) {
>>>  		ret = -EINVAL;
>>>  		goto out;
>>>
>