* [PATCH v2] iommu/vt-d: Fix PASID directory pointer coherency
@ 2023-02-08 0:09 Jacob Pan
2023-02-08 3:46 ` Baolu Lu
0 siblings, 1 reply; 3+ messages in thread
From: Jacob Pan @ 2023-02-08 0:09 UTC (permalink / raw)
To: LKML, iommu, Lu Baolu, Joerg Roedel
Cc: Robin Murphy, Will Deacon, David Woodhouse, Raj Ashok,
Tian, Kevin, Yi Liu, Jacob Pan, stable, Sukumar Ghorai
On platforms that do not support IOMMU Extended capability bit 0
Page-walk Coherency, CPU caches are not snooped when IOMMU is accessing
any translation structures. IOMMU access goes only directly to
memory. Intel IOMMU code was missing a flush for the PASID table
directory that resulted in the unrecoverable fault as shown below.
This patch adds clflush calls whenever activating and updating
a PASID table directory to ensure cache coherency.
On the reverse direction, there's no need to clflush the PASID directory
pointer when we deactivate a context entry in that IOMMU hardware will
not see the old PASID directory pointer after we clear the context entry.
PASID directory entries are also never freed once allocated.
[ 0.555386] DMAR: DRHD: handling fault status reg 3
[ 0.555805] DMAR: [DMA Read NO_PASID] Request device [00:0d.2] fault addr 0x1026a4000 [fault reason 0x51] SM: Present bit in Directory Entry is clear
[ 0.556348] DMAR: Dump dmar1 table entries for IOVA 0x1026a4000
[ 0.556348] DMAR: scalable mode root entry: hi 0x0000000102448001, low 0x0000000101b3e001
[ 0.556348] DMAR: context entry: hi 0x0000000000000000, low 0x0000000101b4d401
[ 0.556348] DMAR: pasid dir entry: 0x0000000101b4e001
[ 0.556348] DMAR: pasid table entry[0]: 0x0000000000000109
[ 0.556348] DMAR: pasid table entry[1]: 0x0000000000000001
[ 0.556348] DMAR: pasid table entry[2]: 0x0000000000000000
[ 0.556348] DMAR: pasid table entry[3]: 0x0000000000000000
[ 0.556348] DMAR: pasid table entry[4]: 0x0000000000000000
[ 0.556348] DMAR: pasid table entry[5]: 0x0000000000000000
[ 0.556348] DMAR: pasid table entry[6]: 0x0000000000000000
[ 0.556348] DMAR: pasid table entry[7]: 0x0000000000000000
[ 0.556348] DMAR: PTE not present at level 4
Cc: <stable@vger.kernel.org>
Fixes: 0bbeb01a4faf ("iommu/vt-d: Manage scalalble mode PASID tables")
Reported-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
v2: Add clflush to PASID directory update case (Baolu, Kevin review)
---
drivers/iommu/intel/iommu.c | 2 ++
drivers/iommu/intel/pasid.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 59df7e42fd53..161342e7149d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1976,6 +1976,8 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
pds = context_get_sm_pds(table);
context->lo = (u64)virt_to_phys(table->table) |
context_pdts(pds);
+ if (!ecap_coherent(iommu->ecap))
+ clflush_cache_range(table->table, sizeof(u64));
/* Setup the RID_PASID field: */
context_set_sm_rid2pasid(context, PASID_RID2PASID);
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index fb3c7020028d..bcb2e6f23742 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -216,6 +216,8 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid)
goto retry;
}
}
+ if (!ecap_coherent(info->iommu->ecap))
+ clflush_cache_range(&dir[dir_index], sizeof(u64));
return &entries[index];
}
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] iommu/vt-d: Fix PASID directory pointer coherency
2023-02-08 0:09 [PATCH v2] iommu/vt-d: Fix PASID directory pointer coherency Jacob Pan
@ 2023-02-08 3:46 ` Baolu Lu
2023-02-08 16:35 ` Jacob Pan
0 siblings, 1 reply; 3+ messages in thread
From: Baolu Lu @ 2023-02-08 3:46 UTC (permalink / raw)
To: Jacob Pan, LKML, iommu, Joerg Roedel
Cc: baolu.lu, Robin Murphy, Will Deacon, David Woodhouse, Raj Ashok,
Tian, Kevin, Yi Liu, stable, Sukumar Ghorai
On 2023/2/8 8:09, Jacob Pan wrote:
> On platforms that do not support IOMMU Extended capability bit 0
> Page-walk Coherency, CPU caches are not snooped when IOMMU is accessing
> any translation structures. IOMMU access goes only directly to
> memory. Intel IOMMU code was missing a flush for the PASID table
> directory that resulted in the unrecoverable fault as shown below.
>
> This patch adds clflush calls whenever activating and updating
> a PASID table directory to ensure cache coherency.
>
> On the reverse direction, there's no need to clflush the PASID directory
> pointer when we deactivate a context entry in that IOMMU hardware will
> not see the old PASID directory pointer after we clear the context entry.
> PASID directory entries are also never freed once allocated.
>
> [ 0.555386] DMAR: DRHD: handling fault status reg 3
> [ 0.555805] DMAR: [DMA Read NO_PASID] Request device [00:0d.2] fault addr 0x1026a4000 [fault reason 0x51] SM: Present bit in Directory Entry is clear
> [ 0.556348] DMAR: Dump dmar1 table entries for IOVA 0x1026a4000
> [ 0.556348] DMAR: scalable mode root entry: hi 0x0000000102448001, low 0x0000000101b3e001
> [ 0.556348] DMAR: context entry: hi 0x0000000000000000, low 0x0000000101b4d401
> [ 0.556348] DMAR: pasid dir entry: 0x0000000101b4e001
> [ 0.556348] DMAR: pasid table entry[0]: 0x0000000000000109
> [ 0.556348] DMAR: pasid table entry[1]: 0x0000000000000001
> [ 0.556348] DMAR: pasid table entry[2]: 0x0000000000000000
> [ 0.556348] DMAR: pasid table entry[3]: 0x0000000000000000
> [ 0.556348] DMAR: pasid table entry[4]: 0x0000000000000000
> [ 0.556348] DMAR: pasid table entry[5]: 0x0000000000000000
> [ 0.556348] DMAR: pasid table entry[6]: 0x0000000000000000
> [ 0.556348] DMAR: pasid table entry[7]: 0x0000000000000000
> [ 0.556348] DMAR: PTE not present at level 4
>
> Cc:<stable@vger.kernel.org>
> Fixes: 0bbeb01a4faf ("iommu/vt-d: Manage scalalble mode PASID tables")
> Reported-by: Sukumar Ghorai<sukumar.ghorai@intel.com>
> Signed-off-by: Ashok Raj<ashok.raj@intel.com>
> Signed-off-by: Jacob Pan<jacob.jun.pan@linux.intel.com>
> ---
> v2: Add clflush to PASID directory update case (Baolu, Kevin review)
> ---
> drivers/iommu/intel/iommu.c | 2 ++
> drivers/iommu/intel/pasid.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 59df7e42fd53..161342e7149d 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1976,6 +1976,8 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
> pds = context_get_sm_pds(table);
> context->lo = (u64)virt_to_phys(table->table) |
> context_pdts(pds);
> + if (!ecap_coherent(iommu->ecap))
> + clflush_cache_range(table->table, sizeof(u64));
This leaves other pasid dir entries not clflush'ed. It is possible that
IOMMU hardware sees different value from what CPU has set. This may
leave security holes for malicious devices. It's same to the pasid entry
table.
How about below change? It does clflush whenever CPU changes the pasid
dir/entry tables.
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index fb3c7020028d..aeb0517826a2 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -128,6 +128,9 @@ int intel_pasid_alloc_table(struct device *dev)
pasid_table->max_pasid = 1 << (order + PAGE_SHIFT + 3);
info->pasid_table = pasid_table;
+ if (!ecap_coherent(info->iommu->ecap))
+ clflush_cache_range(pasid_table->table, size);
+
return 0;
}
@@ -215,6 +218,11 @@ static struct pasid_entry
*intel_pasid_get_entry(struct device *dev, u32 pasid)
free_pgtable_page(entries);
goto retry;
}
+
+ if (!ecap_coherent(info->iommu->ecap)) {
+ clflush_cache_range(entries, VTD_PAGE_SIZE);
+ clflush_cache_range(&dir[dir_index].val,
sizeof(*dir));
+ }
}
return &entries[index];
Best regards,
baolu
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] iommu/vt-d: Fix PASID directory pointer coherency
2023-02-08 3:46 ` Baolu Lu
@ 2023-02-08 16:35 ` Jacob Pan
0 siblings, 0 replies; 3+ messages in thread
From: Jacob Pan @ 2023-02-08 16:35 UTC (permalink / raw)
To: Baolu Lu
Cc: LKML, iommu, Joerg Roedel, Robin Murphy, Will Deacon,
David Woodhouse, Raj Ashok, Tian, Kevin, Yi Liu, stable,
Sukumar Ghorai, jacob.jun.pan
Hi Baolu,
On Wed, 8 Feb 2023 11:46:37 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:
> On 2023/2/8 8:09, Jacob Pan wrote:
> > On platforms that do not support IOMMU Extended capability bit 0
> > Page-walk Coherency, CPU caches are not snooped when IOMMU is accessing
> > any translation structures. IOMMU access goes only directly to
> > memory. Intel IOMMU code was missing a flush for the PASID table
> > directory that resulted in the unrecoverable fault as shown below.
> >
> > This patch adds clflush calls whenever activating and updating
> > a PASID table directory to ensure cache coherency.
> >
> > On the reverse direction, there's no need to clflush the PASID directory
> > pointer when we deactivate a context entry in that IOMMU hardware will
> > not see the old PASID directory pointer after we clear the context
> > entry. PASID directory entries are also never freed once allocated.
> >
> > [ 0.555386] DMAR: DRHD: handling fault status reg 3
> > [ 0.555805] DMAR: [DMA Read NO_PASID] Request device [00:0d.2] fault
> > addr 0x1026a4000 [fault reason 0x51] SM: Present bit in Directory Entry
> > is clear [ 0.556348] DMAR: Dump dmar1 table entries for IOVA
> > 0x1026a4000 [ 0.556348] DMAR: scalable mode root entry: hi
> > 0x0000000102448001, low 0x0000000101b3e001 [ 0.556348] DMAR: context
> > entry: hi 0x0000000000000000, low 0x0000000101b4d401 [ 0.556348]
> > DMAR: pasid dir entry: 0x0000000101b4e001 [ 0.556348] DMAR: pasid
> > table entry[0]: 0x0000000000000109 [ 0.556348] DMAR: pasid table
> > entry[1]: 0x0000000000000001 [ 0.556348] DMAR: pasid table entry[2]:
> > 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[3]:
> > 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[4]:
> > 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[5]:
> > 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[6]:
> > 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[7]:
> > 0x0000000000000000 [ 0.556348] DMAR: PTE not present at level 4
> >
> > Cc:<stable@vger.kernel.org>
> > Fixes: 0bbeb01a4faf ("iommu/vt-d: Manage scalalble mode PASID tables")
> > Reported-by: Sukumar Ghorai<sukumar.ghorai@intel.com>
> > Signed-off-by: Ashok Raj<ashok.raj@intel.com>
> > Signed-off-by: Jacob Pan<jacob.jun.pan@linux.intel.com>
> > ---
> > v2: Add clflush to PASID directory update case (Baolu, Kevin review)
> > ---
> > drivers/iommu/intel/iommu.c | 2 ++
> > drivers/iommu/intel/pasid.c | 2 ++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 59df7e42fd53..161342e7149d 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -1976,6 +1976,8 @@ static int domain_context_mapping_one(struct
> > dmar_domain *domain, pds = context_get_sm_pds(table);
> > context->lo = (u64)virt_to_phys(table->table) |
> > context_pdts(pds);
> > + if (!ecap_coherent(iommu->ecap))
> > + clflush_cache_range(table->table,
> > sizeof(u64));
>
> This leaves other pasid dir entries not clflush'ed. It is possible that
> IOMMU hardware sees different value from what CPU has set. This may
> leave security holes for malicious devices. It's same to the pasid entry
> table.
agreed, we need to address security and functional aspects. good point.
thanks
Jacob
> How about below change? It does clflush whenever CPU changes the pasid
> dir/entry tables.
>
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index fb3c7020028d..aeb0517826a2 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -128,6 +128,9 @@ int intel_pasid_alloc_table(struct device *dev)
> pasid_table->max_pasid = 1 << (order + PAGE_SHIFT + 3);
> info->pasid_table = pasid_table;
>
> + if (!ecap_coherent(info->iommu->ecap))
> + clflush_cache_range(pasid_table->table, size);
> +
> return 0;
> }
>
> @@ -215,6 +218,11 @@ static struct pasid_entry
> *intel_pasid_get_entry(struct device *dev, u32 pasid)
> free_pgtable_page(entries);
> goto retry;
> }
> +
> + if (!ecap_coherent(info->iommu->ecap)) {
> + clflush_cache_range(entries, VTD_PAGE_SIZE);
> + clflush_cache_range(&dir[dir_index].val,
> sizeof(*dir));
> + }
> }
>
> return &entries[index];
>
> Best regards,
> baolu
Thanks,
Jacob
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-02-08 16:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-08 0:09 [PATCH v2] iommu/vt-d: Fix PASID directory pointer coherency Jacob Pan
2023-02-08 3:46 ` Baolu Lu
2023-02-08 16:35 ` Jacob Pan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox