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

[Liu, Yi L]

Eric,

> From: Auger Eric <eric.auger@xxxxxxxxxx>
> Sent: Monday, July 20, 2020 8:38 PM
> 
> 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 ;-)

oh, I think should be using dmar_domain->ioasid_sid from the beginning.
does it look good so far? :-)

Regards,
Yi Liu

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