Message ID | 1595917664-33276-10-git-send-email-yi.l.liu@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | vfio: expose virtual Shared Virtual Addressing to VMs | expand |
Hi Yi, On 7/28/20 8:27 AM, 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 passed from user-space. > e.g. page table bind/unbind and PASID related cache invalidation. > > Cc: Kevin Tian <kevin.tian@intel.com> > CC: Jacob Pan <jacob.jun.pan@linux.intel.com> > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: Eric Auger <eric.auger@redhat.com> > Cc: Jean-Philippe Brucker <jean-philippe@linaro.org> > Cc: Joerg Roedel <joro@8bytes.org> > Cc: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Liu Yi L <yi.l.liu@intel.com> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > --- > 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 b2fe54e..88f4647 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 c85b8d5..b9b29ad 100644 > --- a/drivers/iommu/intel/svm.c > +++ b/drivers/iommu/intel/svm.c > @@ -323,7 +323,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); > + svm = ioasid_find(dmar_domain->ioasid_sid, data->hpasid, NULL); A question about the locking strategy. We don't take the device_domain_lock here. Could you clarify whether it is safe? > if (IS_ERR(svm)) { > ret = PTR_ERR(svm); > goto out; > @@ -440,6 +440,7 @@ int intel_svm_unbind_gpasid(struct iommu_domain *domain, > struct device *dev, u32 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; > @@ -447,8 +448,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); same here. > if (!svm) { > ret = -EINVAL; > goto out; > Thanks Eric
Hi Eric, > From: Auger Eric <eric.auger@redhat.com> > Sent: Sunday, August 16, 2020 12:30 AM > > Hi Yi, > > On 7/28/20 8:27 AM, 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 passed from user-space. > > e.g. page table bind/unbind and PASID related cache invalidation. > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > CC: Jacob Pan <jacob.jun.pan@linux.intel.com> > > Cc: Alex Williamson <alex.williamson@redhat.com> > > Cc: Eric Auger <eric.auger@redhat.com> > > Cc: Jean-Philippe Brucker <jean-philippe@linaro.org> > > Cc: Joerg Roedel <joro@8bytes.org> > > Cc: Lu Baolu <baolu.lu@linux.intel.com> > > Signed-off-by: Liu Yi L <yi.l.liu@intel.com> > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > > --- > > 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 b2fe54e..88f4647 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 c85b8d5..b9b29ad 100644 > > --- a/drivers/iommu/intel/svm.c > > +++ b/drivers/iommu/intel/svm.c > > @@ -323,7 +323,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); > > + svm = ioasid_find(dmar_domain->ioasid_sid, data->hpasid, NULL); > A question about the locking strategy. We don't take the > device_domain_lock here. Could you clarify whether it is safe? I guess it is better to take the same lock as what iommu_domain_set_attr() takes. thanks for catching it. :-) > > > if (IS_ERR(svm)) { > > ret = PTR_ERR(svm); > > goto out; > > @@ -440,6 +440,7 @@ int intel_svm_unbind_gpasid(struct iommu_domain > *domain, > > struct device *dev, u32 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; > > @@ -447,8 +448,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); > same here. same. Regards, Yi Liu > > if (!svm) { > > ret = -EINVAL; > > goto out; > > > Thanks > > Eric
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index b2fe54e..88f4647 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 c85b8d5..b9b29ad 100644 --- a/drivers/iommu/intel/svm.c +++ b/drivers/iommu/intel/svm.c @@ -323,7 +323,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); + svm = ioasid_find(dmar_domain->ioasid_sid, data->hpasid, NULL); if (IS_ERR(svm)) { ret = PTR_ERR(svm); goto out; @@ -440,6 +440,7 @@ int intel_svm_unbind_gpasid(struct iommu_domain *domain, struct device *dev, u32 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; @@ -447,8 +448,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); if (!svm) { ret = -EINVAL; goto out;