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