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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a6b5018a-4231-23ba-cf21-374a91563e43@linux.intel.com>
Date:   Fri, 8 May 2020 10:49:36 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>,
        Joerg Roedel <joro@...tes.org>
Cc:     baolu.lu@...ux.intel.com, "Raj, Ashok" <ashok.raj@...el.com>,
        "jacob.jun.pan@...ux.intel.com" <jacob.jun.pan@...ux.intel.com>,
        "Liu, Yi L" <yi.l.liu@...el.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] iommu/vt-d: Disable non-recoverable fault
 processing before unbind

On 5/8/20 10:12 AM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@...ux.intel.com>
>> Sent: Thursday, May 7, 2020 9:23 PM
>>
>> Hi Kevin,
>>
>> On 2020/5/7 13:45, Tian, Kevin wrote:
>>>> From: Lu Baolu<baolu.lu@...ux.intel.com>
>>>> 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@...ux.intel.com>
>>>> ---
>>>>    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.

I mean, protect the system from malicious devices. In any case, IOMMU
should report and log the unrecoverable faults caused by the untrusted
devices.

Best regards,
baolu

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