[MODERATED] [PATCH 0/4] INVERTv1 2

[MODERATED] [PATCH 0/4] INVERTv1 2

[Andi Kleen]

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

-- 
2.17.1

[MODERATED] [PATCH 1/4] INVERTv1 1

[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 <ak@linux.intel.com>
---
 arch/x86/include/asm/pgtable-invert.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/pgtable-invert.h b/arch/x86/include/asm/pgtable-invert.h
index 177564187fc0..619e35120ed3 100644
--- a/arch/x86/include/asm/pgtable-invert.h
+++ b/arch/x86/include/asm/pgtable-invert.h
@@ -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. */
-- 
2.17.1

[MODERATED] [PATCH 2/4] INVERTv1 4

[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 <ak@linux.intel.com>
---
 arch/x86/include/asm/pgtable.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 26fd42a91946..4e612062bd5a 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -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)
-- 
2.17.1

[MODERATED] [PATCH 3/4] INVERTv1 3

[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 <ak@linux.intel.com>
---
 arch/x86/mm/kmmio.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/x86/mm/kmmio.c b/arch/x86/mm/kmmio.c
index 7c8686709636..a4c0519f9490 100644
--- a/arch/x86/mm/kmmio.c
+++ b/arch/x86/mm/kmmio.c
@@ -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)
-- 
2.17.1

[MODERATED] [PATCH 4/4] INVERTv1 0

[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 <ak@linux.intel.com>
---
 arch/x86/mm/pageattr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 3bded76e8d5c..7bb6f65c79de 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -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;
-- 
2.17.1

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

[Linus Torvalds]

On Mon, 6 Aug 2018, speck for Andi Kleen wrote:
> 
> 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.

>  static inline bool __pte_needs_invert(u64 val)
>  {
> -	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

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

[Andi Kleen]

On Mon, Aug 06, 2018 at 04:36:29PM -0700, speck for Linus Torvalds wrote:
> 
> 
> On Mon, 6 Aug 2018, speck for Andi Kleen wrote:
> > 
> > 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.
> 
> >  static inline bool __pte_needs_invert(u64 val)
> >  {
> > -	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?

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

[MODERATED] [PATCH 1/4] INVERTv2 1

[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 <ak@linux.intel.com>

---
v2: Actually use correct check (Linus)
---
 arch/x86/include/asm/pgtable-invert.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/pgtable-invert.h b/arch/x86/include/asm/pgtable-invert.h
index 177564187fc0..6ceb0cd968e3 100644
--- a/arch/x86/include/asm/pgtable-invert.h
+++ b/arch/x86/include/asm/pgtable-invert.h
@@ -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. */
-- 
2.17.1

[MODERATED] [PATCH 2/4] INVERTv2 4

[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 <ak@linux.intel.com>
---
 arch/x86/include/asm/pgtable.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 26fd42a91946..4e612062bd5a 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -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)
-- 
2.17.1

[MODERATED] [PATCH 3/4] INVERTv2 3

[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 <ak@linux.intel.com>
---
 arch/x86/mm/kmmio.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/x86/mm/kmmio.c b/arch/x86/mm/kmmio.c
index 7c8686709636..a4c0519f9490 100644
--- a/arch/x86/mm/kmmio.c
+++ b/arch/x86/mm/kmmio.c
@@ -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)
-- 
2.17.1

[MODERATED] [PATCH 4/4] INVERTv2 0

[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 <ak@linux.intel.com>
---
 arch/x86/mm/pageattr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 3bded76e8d5c..7bb6f65c79de 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -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;
-- 
2.17.1

Re: [PATCH 3/4] INVERTv2 3

[Thomas Gleixner]

On Tue, 7 Aug 2018, speck for Andi Kleen wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 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

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

[Andi Kleen]

On Wed, Aug 08, 2018 at 09:23:17AM +0200, speck for Thomas Gleixner wrote:
> On Tue, 7 Aug 2018, speck for Andi Kleen wrote:
> 
> > From: Andi Kleen <ak@linux.intel.com>
> > 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.

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

Re: [PATCH 3/4] INVERTv2 3

[Thomas Gleixner]

On Wed, 8 Aug 2018, speck for Andi Kleen wrote:

> On Wed, Aug 08, 2018 at 09:23:17AM +0200, speck for Thomas Gleixner wrote:
> > On Tue, 7 Aug 2018, speck for Andi Kleen wrote:
> > 
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 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.
> 
> 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!