Skip to contentSkip to conversation

INVERTv1 2

PATCH In review Andi Kleen 4 participants 14 messages Aug 6, 2018 – Aug 8, 2018

Subsystems (3)
x86 x86 MM Tracing MMIO accesses (mmiotrace)
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

cover INVERTv1 2

Dave Hansen pointed out that set_memory_np didn't invert its non present PTEs. During another audit we found also a case in THP that didn't invert.

This patch kit fixes inversion for those kernel mappings.

Mostly it's interesting for guest kernels when it would allow bypassing EPT inside the same guest.

For production binary kernels without debug options only the first two patches are needed.

Andi Kleen (4):
x86/speculation/l1tf: Enable invertion for non PROTNONE !P maps x86/speculation/l1tf: Make pmd/pud_mknotpresent invert x86/speculation/l1tf: Make the mmio tracer invert x86/speculation/l1tf: Make set_memory_np invert

 arch/x86/include/asm/pgtable-invert.h |  2 +-
 arch/x86/include/asm/pgtable.h        | 22 ++++++++++++----------
 arch/x86/mm/kmmio.c                   | 23 +++++++++++++----------
 arch/x86/mm/pageattr.c                |  8 ++++----
 4 files changed, 30 insertions(+), 25 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 1/4 INVERTv1 1

For kernel mappings PAGE_PROTNONE is not necessarily set for non present mapping. To enable PTE inversion for those mappings enable it for any non present mapping, not just prot_none.

Signed-off-by Andi Kleen <[email protected]>
---
 arch/x86/include/asm/pgtable-invert.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/include/asm/pgtable-invert.h +1 -1
index 177564187fc0..619e35120ed3 100644
@@ -6,7 +6,7 @@
 
 static inline bool __pte_needs_invert(u64 val)
 {
-	return (val & (_PAGE_PRESENT|_PAGE_PROTNONE)) == _PAGE_PROTNONE;
+	return (val & (_PAGE_PRESENT|_PAGE_PROTNONE)) == 0;
 }
 
 /* Get a mask to xor with the page table entry to get the correct pfn. */
Signature · 1 line
2.17.1
#2 lore raw

[MODERATED] Re: [PATCH 1/4] INVERTv1 1

Quoted 4 lines · speck for Andi Kleen

For kernel mappings PAGE_PROTNONE is not necessarily set for non present mapping. To enable PTE inversion for those mappings enable it for any non present mapping, not just prot_none.

Your patch does not match your description.

arch/x86/include/asm/pgtable-invert.h line 10

Quoted 5 lines

static inline bool __pte_needs_invert(u64 val)
{

arch/x86/include/asm/pgtable-invert.h +1 -1
-	return (val & (_PAGE_PRESENT|_PAGE_PROTNONE)) == _PAGE_PROTNONE;
+	return (val & (_PAGE_PRESENT|_PAGE_PROTNONE)) == 0;
 }

That patch now makes it *not* trigger for _PAGE_PROTNONE pages.

Did you perhaps mean

        return !(val & _PAGE_PRESENT);

instead?

Linus

#3 lore raw

[MODERATED] Re: [PATCH 1/4] INVERTv1 1

Quoted 18 lines · speck for Linus Torvalds
Quoted 4 lines · speck for Andi Kleen

For kernel mappings PAGE_PROTNONE is not necessarily set for non present mapping. To enable PTE inversion for those mappings enable it for any non present mapping, not just prot_none.

Your patch does not match your description.

arch/x86/include/asm/pgtable-invert.h line 10

Quoted 5 lines

static inline bool __pte_needs_invert(u64 val)
{

arch/x86/include/asm/pgtable-invert.h +1 -1
-	return (val & (_PAGE_PRESENT|_PAGE_PROTNONE)) == _PAGE_PROTNONE;
+	return (val & (_PAGE_PRESENT|_PAGE_PROTNONE)) == 0;
 }

That patch now makes it *not* trigger for _PAGE_PROTNONE pages.

Quoted 5 lines

Did you perhaps mean

        return !(val & _PAGE_PRESENT);

instead?

Yes you're right of course. Putting on the brown paperbag. That's what you get from last minute changes :/

Will send a new version.

Thanks,

-Andi

author
#4 lore raw

PATCH 1/4 INVERTv2 1

In reply to Andi Kleen

For kernel mappings PAGE_PROTNONE is not necessarily set for non present mapping. To enable PTE inversion for those mappings enable it for any non present mapping, not just prot_none.

Signed-off-by Andi Kleen <[email protected]>

---
v2: Actually use correct check (Linus)

---
 arch/x86/include/asm/pgtable-invert.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/include/asm/pgtable-invert.h +1 -1
index 177564187fc0..6ceb0cd968e3 100644
@@ -6,7 +6,7 @@
 
 static inline bool __pte_needs_invert(u64 val)
 {
-	return (val & (_PAGE_PRESENT|_PAGE_PROTNONE)) == _PAGE_PROTNONE;
+	return (val & _PAGE_PRESENT) == 0;
 }
 
 /* Get a mask to xor with the page table entry to get the correct pfn. */
Signature · 1 line
2.17.1
author
#5 lore raw

PATCH 2/4 INVERTv1 4

In reply to Andi Kleen

Some cases in THP:
- MADV_FREE
- mprotect
- split
invalidate the PMD for a short time to avoid races. Make sure the PMD is inverted while this happens too. The window for an L1TF attack here is very for, but better to close it.

We fix pmd/pud_mknotpresent to use the right low level functions to correctly invert.

pud_mknotpresent is not used by anything, but fixed too.

Signed-off-by Andi Kleen <[email protected]>
---
 arch/x86/include/asm/pgtable.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
arch/x86/include/asm/pgtable.h +12 -10
index 26fd42a91946..4e612062bd5a 100644
@@ -410,11 +410,6 @@ static inline pmd_t pmd_mkwrite(pmd_t pmd)
 	return pmd_set_flags(pmd, _PAGE_RW);
 }
 
-static inline pmd_t pmd_mknotpresent(pmd_t pmd)
-{
-	return pmd_clear_flags(pmd, _PAGE_PRESENT | _PAGE_PROTNONE);
-}
-
 static inline pud_t pud_set_flags(pud_t pud, pudval_t set)
 {
 	pudval_t v = native_pud_val(pud);

@@ -469,11 +464,6 @@ static inline pud_t pud_mkwrite(pud_t pud)
 	return pud_set_flags(pud, _PAGE_RW);
 }
 
-static inline pud_t pud_mknotpresent(pud_t pud)
-{
-	return pud_clear_flags(pud, _PAGE_PRESENT | _PAGE_PROTNONE);
-}
-
 #ifdef CONFIG_HAVE_ARCH_SOFT_DIRTY
 static inline int pte_soft_dirty(pte_t pte)
 {

@@ -577,6 +567,18 @@ static inline pud_t pfn_pud(unsigned long page_nr, pgprot_t pgprot)
 	return __pud(pfn | check_pgprot(pgprot));
 }
 
+static inline pmd_t pmd_mknotpresent(pmd_t pmd)
+{
+	return pfn_pmd(pmd_pfn(pmd),
+		      __pgprot(pmd_flags(pmd) & ~(_PAGE_PRESENT|_PAGE_PROTNONE)));
+}
+
+static inline pud_t pud_mknotpresent(pud_t pud)
+{
+	return pfn_pud(pud_pfn(pud),
+	      __pgprot(pud_flags(pud) & ~(_PAGE_PRESENT|_PAGE_PROTNONE)));
+}
+
 static inline u64 flip_protnone_guard(u64 oldval, u64 val, u64 mask);
 
 static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
Signature · 1 line
2.17.1
author
#6 lore raw

PATCH 2/4 INVERTv2 4

In reply to Andi Kleen

Some cases in THP like:
- MADV_FREE
- mprotect
- split
invalidate the PMD for a short time to avoid races.

Make sure the PMD is inverted while this happens too. The window for an L1TF attack here is very small, but better to close it.

We fix pmd/pud_mknotpresent to use the right low level functions to correctly invert.

pud_mknotpresent is not used by anything, but fixed too.

Signed-off-by Andi Kleen <[email protected]>
---
 arch/x86/include/asm/pgtable.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
arch/x86/include/asm/pgtable.h +12 -10
index 26fd42a91946..4e612062bd5a 100644
@@ -410,11 +410,6 @@ static inline pmd_t pmd_mkwrite(pmd_t pmd)
 	return pmd_set_flags(pmd, _PAGE_RW);
 }
 
-static inline pmd_t pmd_mknotpresent(pmd_t pmd)
-{
-	return pmd_clear_flags(pmd, _PAGE_PRESENT | _PAGE_PROTNONE);
-}
-
 static inline pud_t pud_set_flags(pud_t pud, pudval_t set)
 {
 	pudval_t v = native_pud_val(pud);

@@ -469,11 +464,6 @@ static inline pud_t pud_mkwrite(pud_t pud)
 	return pud_set_flags(pud, _PAGE_RW);
 }
 
-static inline pud_t pud_mknotpresent(pud_t pud)
-{
-	return pud_clear_flags(pud, _PAGE_PRESENT | _PAGE_PROTNONE);
-}
-
 #ifdef CONFIG_HAVE_ARCH_SOFT_DIRTY
 static inline int pte_soft_dirty(pte_t pte)
 {

@@ -577,6 +567,18 @@ static inline pud_t pfn_pud(unsigned long page_nr, pgprot_t pgprot)
 	return __pud(pfn | check_pgprot(pgprot));
 }
 
+static inline pmd_t pmd_mknotpresent(pmd_t pmd)
+{
+	return pfn_pmd(pmd_pfn(pmd),
+		      __pgprot(pmd_flags(pmd) & ~(_PAGE_PRESENT|_PAGE_PROTNONE)));
+}
+
+static inline pud_t pud_mknotpresent(pud_t pud)
+{
+	return pfn_pud(pud_pfn(pud),
+	      __pgprot(pud_flags(pud) & ~(_PAGE_PRESENT|_PAGE_PROTNONE)));
+}
+
 static inline u64 flip_protnone_guard(u64 oldval, u64 val, u64 mask);
 
 static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
Signature · 1 line
2.17.1
author
#7 lore raw

PATCH 3/4 INVERTv1 3

In reply to Andi Kleen

The mmio tracer sets io mapping PTEs and PMDs to non present when enabled. Make it use the right low level macros to actually invert them to protect against L1TF.

Unfortunately I wasn't able to test this, since the procedure described in Documentation/trace/mmiotrace.rst doesn't result in any tracing activity, with or without my patch, when starting X.
Since it's unlikely that mmio tracing is used in any production environment it might be reasonable to leave it alone. However the patch is simple enough that it may be still acceptable.

Signed-off-by Andi Kleen <[email protected]>
---
 arch/x86/mm/kmmio.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)
arch/x86/mm/kmmio.c +13 -10
index 7c8686709636..a4c0519f9490 100644
@@ -126,24 +126,27 @@ static struct kmmio_fault_page *get_kmmio_fault_page(unsigned long addr)
 
 static void clear_pmd_presence(pmd_t *pmd, bool clear, pmdval_t *old)
 {
+	pmd_t new_pmd;
 	pmdval_t v = pmd_val(*pmd);
 	if (clear) {
-		*old = v & _PAGE_PRESENT;
-		v &= ~_PAGE_PRESENT;
-	} else	/* presume this has been called with clear==true previously */
-		v |= *old;
-	set_pmd(pmd, __pmd(v));
+		*old = v;
+		new_pmd = pmd_mknotpresent(*pmd);
+	} else { /* presume this has been called with clear==true previously */
+		new_pmd = __pmd(*old);
+	}
+	set_pmd(pmd, new_pmd);
 }
 
 static void clear_pte_presence(pte_t *pte, bool clear, pteval_t *old)
 {
 	pteval_t v = pte_val(*pte);
 	if (clear) {
-		*old = v & _PAGE_PRESENT;
-		v &= ~_PAGE_PRESENT;
-	} else	/* presume this has been called with clear==true previously */
-		v |= *old;
-	set_pte_atomic(pte, __pte(v));
+		*old = v;
+		/* nothing should care about address */
+		pte_clear(&init_mm, 0, pte);
+	} else { /* presume this has been called with clear==true previously */
+		set_pte_atomic(pte, __pte(*old));
+	}
 }
 
 static int clear_page_presence(struct kmmio_fault_page *f, bool clear)
Signature · 1 line
2.17.1
author
#8 lore raw

PATCH 3/4 INVERTv2 3

In reply to Andi Kleen

The mmio tracer sets io mapping PTEs and PMDs to non present when enabled. Make it use the right low level macros to actually invert them to protect against L1TF.

Unfortunately I wasn't able to test this, since the procedure described in Documentation/trace/mmiotrace.rst doesn't result in any tracing activity, with or without my patch, when starting X.
Since it's unlikely that mmio tracing is used in any production environment it might be reasonable to leave it alone. However the patch is simple enough that it may be still acceptable.

Signed-off-by Andi Kleen <[email protected]>
---
 arch/x86/mm/kmmio.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)
arch/x86/mm/kmmio.c +13 -10
index 7c8686709636..a4c0519f9490 100644
@@ -126,24 +126,27 @@ static struct kmmio_fault_page *get_kmmio_fault_page(unsigned long addr)
 
 static void clear_pmd_presence(pmd_t *pmd, bool clear, pmdval_t *old)
 {
+	pmd_t new_pmd;
 	pmdval_t v = pmd_val(*pmd);
 	if (clear) {
-		*old = v & _PAGE_PRESENT;
-		v &= ~_PAGE_PRESENT;
-	} else	/* presume this has been called with clear==true previously */
-		v |= *old;
-	set_pmd(pmd, __pmd(v));
+		*old = v;
+		new_pmd = pmd_mknotpresent(*pmd);
+	} else { /* presume this has been called with clear==true previously */
+		new_pmd = __pmd(*old);
+	}
+	set_pmd(pmd, new_pmd);
 }
 
 static void clear_pte_presence(pte_t *pte, bool clear, pteval_t *old)
 {
 	pteval_t v = pte_val(*pte);
 	if (clear) {
-		*old = v & _PAGE_PRESENT;
-		v &= ~_PAGE_PRESENT;
-	} else	/* presume this has been called with clear==true previously */
-		v |= *old;
-	set_pte_atomic(pte, __pte(v));
+		*old = v;
+		/* nothing should care about address */
+		pte_clear(&init_mm, 0, pte);
+	} else { /* presume this has been called with clear==true previously */
+		set_pte_atomic(pte, __pte(*old));
+	}
 }
 
 static int clear_page_presence(struct kmmio_fault_page *f, bool clear)
Signature · 1 line
2.17.1
#9 lore raw

Re: [PATCH 3/4] INVERTv2 3

Quoted 10 lines · speck for Andi Kleen

From: Andi Kleen <[email protected]>
Subject: x86/speculation/l1tf: Make the mmio tracer invert

The mmio tracer sets io mapping PTEs and PMDs to non present when enabled. Make it use the right low level macros to actually invert them to protect against L1TF.

Unfortunately I wasn't able to test this, since the procedure described in Documentation/trace/mmiotrace.rst doesn't result in any tracing activity, with or without my patch, when starting X.

You need to make sure that the GFX driver is loaded _after_ enabling MMIO tracing. I'll omit that for now, until we have confirmation about testing.

Thanks,

tglx

#10 lore raw

[MODERATED] Re: [PATCH 3/4] INVERTv2 3

Quoted 9 lines · speck for Thomas Gleixner
Quoted 7 lines · speck for Andi Kleen

From: Andi Kleen <[email protected]>
Subject: x86/speculation/l1tf: Make the mmio tracer invert

The mmio tracer sets io mapping PTEs and PMDs to non present when enabled. Make it use the right low level macros to actually invert them to protect against L1TF.

Quoted 6 lines
Quoted 3 lines

Unfortunately I wasn't able to test this, since the procedure described in Documentation/trace/mmiotrace.rst doesn't result in any tracing activity, with or without my patch, when starting X.

You need to make sure that the GFX driver is loaded _after_ enabling MMIO tracing. I'll omit that for now, until we have confirmation about testing.

It seems very difficult to load the gfx driver late. After wasting a lot of time trying to work around this, I managed to get the testmmiotrace module to work, and it appears the trace is working even with my patches.

So the testing paragraph can be dropped.

-Andi

#11 lore raw

Re: [PATCH 3/4] INVERTv2 3

Quoted 14 lines · speck for Andi Kleen
Quoted 13 lines · speck for Thomas Gleixner
Quoted 10 lines · speck for Andi Kleen

From: Andi Kleen <[email protected]>
Subject: x86/speculation/l1tf: Make the mmio tracer invert

The mmio tracer sets io mapping PTEs and PMDs to non present when enabled. Make it use the right low level macros to actually invert them to protect against L1TF.

Unfortunately I wasn't able to test this, since the procedure described in Documentation/trace/mmiotrace.rst doesn't result in any tracing activity, with or without my patch, when starting X.

Quoted 8 lines
Quoted 2 lines

You need to make sure that the GFX driver is loaded _after_ enabling MMIO tracing. I'll omit that for now, until we have confirmation about testing.

It seems very difficult to load the gfx driver late. After wasting a lot of time trying to work around this, I managed to get the testmmiotrace module to work, and it appears the trace is working even with my patches.

So the testing paragraph can be dropped.

Ok. Thanks!

author
#12 lore raw

PATCH 4/4 INVERTv1 0

In reply to Andi Kleen

Make pageattr set_memory_np invert to protect these kernel mappings against L1TF. This mainly affects DEBUG_PAGEALLOC and other debugging options.

The fix is to use the standard low level macros to set up the PTEs, instead of open coding the PTE manipulation.

Signed-off-by Andi Kleen <[email protected]>
---
 arch/x86/mm/pageattr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/mm/pageattr.c +4 -4
index 3bded76e8d5c..7bb6f65c79de 100644
@@ -1014,8 +1014,8 @@ static long populate_pmd(struct cpa_data *cpa,
 
 		pmd = pmd_offset(pud, start);
 
-		set_pmd(pmd, __pmd(cpa->pfn << PAGE_SHIFT | _PAGE_PSE |
-				   massage_pgprot(pmd_pgprot)));
+		set_pmd(pmd, pmd_mkhuge(pfn_pmd(cpa->pfn,
+					canon_pgprot(pmd_pgprot))));
 
 		start	  += PMD_SIZE;
 		cpa->pfn  += PMD_SIZE >> PAGE_SHIFT;

@@ -1087,8 +1087,8 @@ static int populate_pud(struct cpa_data *cpa, unsigned long start, p4d_t *p4d,
 	 * Map everything starting from the Gb boundary, possibly with 1G pages
 	 */
 	while (boot_cpu_has(X86_FEATURE_GBPAGES) && end - start >= PUD_SIZE) {
-		set_pud(pud, __pud(cpa->pfn << PAGE_SHIFT | _PAGE_PSE |
-				   massage_pgprot(pud_pgprot)));
+		set_pud(pud, pud_mkhuge(pfn_pud(cpa->pfn,
+				   canon_pgprot(pud_pgprot))));
 
 		start	  += PUD_SIZE;
 		cpa->pfn  += PUD_SIZE >> PAGE_SHIFT;
Signature · 1 line
2.17.1
author
#13 lore raw

PATCH 4/4 INVERTv2 0

In reply to Andi Kleen

Make pageattr set_memory_np invert to protect these kernel mappings against L1TF. This mainly affects DEBUG_PAGEALLOC and other debugging options.

The fix is to use the standard low level macros to set up the PTEs, instead of open coding the PTE manipulation.

Passes the CPA self test.

Signed-off-by Andi Kleen <[email protected]>
---
 arch/x86/mm/pageattr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/mm/pageattr.c +4 -4
index 3bded76e8d5c..7bb6f65c79de 100644
@@ -1014,8 +1014,8 @@ static long populate_pmd(struct cpa_data *cpa,
 
 		pmd = pmd_offset(pud, start);
 
-		set_pmd(pmd, __pmd(cpa->pfn << PAGE_SHIFT | _PAGE_PSE |
-				   massage_pgprot(pmd_pgprot)));
+		set_pmd(pmd, pmd_mkhuge(pfn_pmd(cpa->pfn,
+					canon_pgprot(pmd_pgprot))));
 
 		start	  += PMD_SIZE;
 		cpa->pfn  += PMD_SIZE >> PAGE_SHIFT;

@@ -1087,8 +1087,8 @@ static int populate_pud(struct cpa_data *cpa, unsigned long start, p4d_t *p4d,
 	 * Map everything starting from the Gb boundary, possibly with 1G pages
 	 */
 	while (boot_cpu_has(X86_FEATURE_GBPAGES) && end - start >= PUD_SIZE) {
-		set_pud(pud, __pud(cpa->pfn << PAGE_SHIFT | _PAGE_PSE |
-				   massage_pgprot(pud_pgprot)));
+		set_pud(pud, pud_mkhuge(pfn_pud(cpa->pfn,
+				   canon_pgprot(pud_pgprot))));
 
 		start	  += PUD_SIZE;
 		cpa->pfn  += PUD_SIZE >> PAGE_SHIFT;
Signature · 1 line
2.17.1