RE: [PATCH v4 3/5] iommu/vt-d: Disable non-recoverable fault processing before unbind

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Thursday, May 7, 2020 9:23 PM
> 
> Hi Kevin,
> 
> On 2020/5/7 13:45, Tian, Kevin wrote:
> >> From: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> >> Sent: Thursday, May 7, 2020 8:56 AM
> >>
> >> When a PASID is used for SVA by the device, it's possible that the PASID
> >> entry is cleared before the device flushes all ongoing DMA requests. The
> >> IOMMU should ignore the non-recoverable faults caused by these
> requests.
> >> Intel VT-d provides such function through the FPD bit of the PASID entry.
> >> This sets FPD bit when PASID entry is cleared in the mm notifier and
> >> clear it when the pasid is unbound.
> >>
> >> Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> >> ---
> >>   drivers/iommu/intel-iommu.c |  4 ++--
> >>   drivers/iommu/intel-pasid.c | 26 +++++++++++++++++++++-----
> >>   drivers/iommu/intel-pasid.h |  3 ++-
> >>   drivers/iommu/intel-svm.c   |  9 ++++++---
> >>   4 files changed, 31 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> >> index d1866c0905b1..7811422b5a68 100644
> >> --- a/drivers/iommu/intel-iommu.c
> >> +++ b/drivers/iommu/intel-iommu.c
> >> @@ -5352,7 +5352,7 @@ static void
> __dmar_remove_one_dev_info(struct
> >> device_domain_info *info)
> >>   	if (info->dev) {
> >>   		if (dev_is_pci(info->dev) && sm_supported(iommu))
> >>   			intel_pasid_tear_down_entry(iommu, info->dev,
> >> -					PASID_RID2PASID);
> >> +					PASID_RID2PASID, false);
> >>
> >>   		iommu_disable_dev_iotlb(info);
> >>   		domain_context_clear(iommu, info->dev);
> >> @@ -5587,7 +5587,7 @@ static void aux_domain_remove_dev(struct
> >> dmar_domain *domain,
> >>   	auxiliary_unlink_device(domain, dev);
> >>
> >>   	spin_lock(&iommu->lock);
> >> -	intel_pasid_tear_down_entry(iommu, dev, domain->default_pasid);
> >> +	intel_pasid_tear_down_entry(iommu, dev, domain->default_pasid,
> >> false);
> >>   	domain_detach_iommu(domain, iommu);
> >>   	spin_unlock(&iommu->lock);
> >>
> >> diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
> >> index 7969e3dac2ad..11aef6c12972 100644
> >> --- a/drivers/iommu/intel-pasid.c
> >> +++ b/drivers/iommu/intel-pasid.c
> >> @@ -292,7 +292,20 @@ static inline void pasid_clear_entry(struct
> >> pasid_entry *pe)
> >>   	WRITE_ONCE(pe->val[7], 0);
> >>   }
> >>
> >> -static void intel_pasid_clear_entry(struct device *dev, int pasid)
> >> +static inline void pasid_clear_entry_with_fpd(struct pasid_entry *pe)
> >> +{
> >> +	WRITE_ONCE(pe->val[0], PASID_PTE_FPD);
> >> +	WRITE_ONCE(pe->val[1], 0);
> >> +	WRITE_ONCE(pe->val[2], 0);
> >> +	WRITE_ONCE(pe->val[3], 0);
> >> +	WRITE_ONCE(pe->val[4], 0);
> >> +	WRITE_ONCE(pe->val[5], 0);
> >> +	WRITE_ONCE(pe->val[6], 0);
> >> +	WRITE_ONCE(pe->val[7], 0);
> >> +}
> >> +
> >> +static void
> >> +intel_pasid_clear_entry(struct device *dev, int pasid, bool pf_ignore)
> > Hi, Baolu,
> >
> > Just curious whether it makes sense to always set FPD here. Yes, SVA is
> > one known example that non-recoverable fault associated with a PASID
> > entry might be caused after the entry is cleared and those are considered
> > benign.  But even in a general context (w/o SVA) why do we care about
> > such faults after a PASID entry is torn down?
> 
> First level page tables are also used for DMA protection. For example,
> thunderbolt peripherals are always untrusted and should be protected
> with IOMMU. IOMMU should always report unrecoverable faults generated
> by those device to detect possible DMA attacks.

when untrusted devices are protected by IOMMU, I don't think PASID
entry (of RID2PASID) will have present bit cleared.

> 
> ATS/PRI devices are always trusted devices, hence we could tolerate
> setting FPD bit in the time window between mm_release notifier and
> unbind().
> 
> Best regards,
> baolu