linux-kernel - Re: [PATCH v2 2/3] iommu/vt-d: Clear Present bit before tearing down context entry

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <f5b1aa1a-5d20-40c8-9f3f-5bc37766fe75@linux.intel.com>
Date: Wed, 21 Jan 2026 16:04:11 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: "Tian, Kevin" <kevin.tian@...el.com>, Joerg Roedel <joro@...tes.org>,
 Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
 Jason Gunthorpe <jgg@...dia.com>
Cc: Dmytro Maluka <dmaluka@...omium.org>,
 Samiullah Khawaja <skhawaja@...gle.com>,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] iommu/vt-d: Clear Present bit before tearing down
 context entry

On 1/21/26 15:50, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@...ux.intel.com>
>> Sent: Wednesday, January 21, 2026 3:29 PM
>>
>> On 1/21/26 14:23, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@...ux.intel.com>
>>>> Sent: Tuesday, January 20, 2026 2:18 PM
>>>>
>>>> When tearing down a context entry, the current implementation zeros
>> the
>>>> entire 128-bit entry using multiple 64-bit writes. This creates a window
>>>> where the hardware can fetch a "torn" entry — where some fields are
>>>> already zeroed while the 'Present' bit is still set — leading to
>>>> unpredictable behavior or spurious faults.
>>>>
>>>> While x86 provides strong write ordering, the compiler may reorder
>> writes
>>>> to the two 64-bit halves of the context entry. Even without compiler
>>>> reordering, the hardware fetch is not guaranteed to be atomic with
>>>> respect to multiple CPU writes.
>>>>
>>>> Align with the "Guidance to Software for Invalidations" in the VT-d spec
>>>> (Section 6.5.3.3) by implementing the recommended ownership
>> handshake:
>>>>
>>>> 1. Clear only the 'Present' (P) bit of the context entry first to
>>>>      signal the transition of ownership from hardware to software.
>>>> 2. Use dma_wmb() to ensure the cleared bit is visible to the IOMMU.
>>>> 3. Perform the required cache and context-cache invalidation to ensure
>>>>      hardware no longer has cached references to the entry.
>>>> 4. Fully zero out the entry only after the invalidation is complete.
>>>>
>>>> Also, add a dma_wmb() to context_set_present() to ensure the entry
>>>> is fully initialized before the 'Present' bit becomes visible.
>>>>
>>>> Fixes: ba39592764ed2 ("Intel IOMMU: Intel IOMMU driver")
>>>> Reported-by: Dmytro Maluka <dmaluka@...omium.org>
>>>> Closes: https://lore.kernel.org/all/aTG7gc7I5wExai3S@google.com/
>>>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>>>
>>> Reviewed-by: Kevin Tian <kevin.tian@...el.com>
>>>
>>> btw there is a context_clear_entry() for copied context entry in
>>> device_pasid_table_setup(), but this patch doesn't touch that
>>> path. It seems to assume that no in-flight DMA will exist at that
>>> point:
>>>
>>>           if (context_copied(iommu, bus, devfn)) {
>>>                   context_clear_entry(context);
>>>                   ...
>>>                   /*
>>>                    * At this point, the device is supposed to finish reset at
>>>                    * its driver probe stage, so no in-flight DMA will exist,
>>>                    * and we don't need to worry anymore hereafter.
>>>                    */
>>>                   clear_context_copied(iommu, bus, devfn);
>>>
>>> Is that guaranteed by all devices? from kdump feature p.o.v. if
>>> that assumption is broken it just means potential DMA errors
>>> in this transition window. But regarding to the issue which this
>>> patch tries to fix, in-fly DMAs may lead to undesired behaviors
>>> including memory corruption etc.
>>>
>>> So, should it be fixed too?
>>
>> This path is triggered when the device driver has probed the device
>> (ensuring it has been reset) and then calls the kernel DMA API for the
>> first time. At this stage, there should be no in-flight DMAs. We can
>> apply the same logic here to improve code readability, but this is not a
>> bug that requires a fix. Or not?
>>
> 
> device could be in whatever state when kdump is triggered. I'm not
> sure whether all device drivers will reset the device at probe time.

Okay, agreed.

> Just thought that applying the same due diligence here could prevent
> any undesired damage just in case. Not exactly for backporting, but
> it's always good to have consistent logic to avoid special case based
> on subtle assumptions...

So I will add the following additional change:

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 34f4af4e9b5c..b63a71904cfb 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -840,7 +840,7 @@ static int device_pasid_table_setup(struct device 
*dev, u8 bus, u8 devfn)
         }

         if (context_copied(iommu, bus, devfn)) {
-               context_clear_entry(context);
+               context_clear_present(context);
                 __iommu_flush_cache(iommu, context, sizeof(*context));

                 /*
@@ -860,6 +860,9 @@ static int device_pasid_table_setup(struct device 
*dev, u8 bus, u8 devfn)
                 iommu->flush.flush_iotlb(iommu, 0, 0, 0, 
DMA_TLB_GLOBAL_FLUSH);
                 devtlb_invalidation_with_pasid(iommu, dev, IOMMU_NO_PASID);

+               context_clear_entry(context);
+               __iommu_flush_cache(iommu, context, sizeof(*context));
+
                 /*
                  * At this point, the device is supposed to finish reset at
                  * its driver probe stage, so no in-flight DMA will exist,

Appears good to you?

Thanks,
baolu