Add support for eXclusive Page Frame Ownership

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Tycho Andersen
Sent: Thursday, September 7, 2017 10:36 AM
To: [email protected]
Cc: [email protected]; [email protected]; Marco Benatto <[email protected]>; Juerg Haefliger <[email protected]>; [email protected]; Tycho Andersen <[email protected]>
Subject: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame Ownership (XPFO)

From: Juerg Haefliger <[email protected]>

This patch adds support for XPFO which protects against 'ret2dir' kernel attacks. The basic idea is to enforce exclusive ownership of page frames by either the kernel or userspace, unless explicitly requested by the kernel. Whenever a page destined for userspace is allocated, it is unmapped from physmap (the kernel's page table). When such a page is reclaimed from userspace, it is mapped back to physmap.

Additional fields in the page_ext struct are used for XPFO housekeeping, specifically:
- two flags to distinguish user vs. kernel pages and to tag unmapped pages.
- a reference counter to balance kmap/kunmap operations.
- a lock to serialize access to the XPFO fields.

This patch is based on the work of Vasileios P. Kemerlis et al. who published their work in this paper:
http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf

v6: * use flush_tlb_kernel_range() instead of __flush_tlb_one, so we flush
the tlb entry on all CPUs when unmapping it in kunmap
* handle lookup_page_ext()/lookup_xpfo() returning NULL
* drop lots of BUG()s in favor of WARN()
* don't disable irqs in xpfo_kmap/xpfo_kunmap, export __split_large_page so we can do our own alloc_pages(GFP_ATOMIC) to pass it

CC [email protected]

Suggested-by Vasileios P. Kemerlis <[email protected]>

Signed-off-by Juerg Haefliger <[email protected]>

Signed-off-by Tycho Andersen <[email protected]>

Signed-off-by Marco Benatto <[email protected]>

---
 Documentation/admin-guide/kernel-parameters.txt |   2 +
 arch/x86/Kconfig                                |   1 +
 arch/x86/include/asm/pgtable.h                  |  25 +++
 arch/x86/mm/Makefile                            |   1 +
 arch/x86/mm/pageattr.c                          |  22 +--
 arch/x86/mm/xpfo.c                              | 114 ++++++++++++
 include/linux/highmem.h                         |  15 +-
 include/linux/xpfo.h                            |  42 +++++
 mm/Makefile                                     |   1 +
 mm/page_alloc.c                                 |   2 +
 mm/page_ext.c                                   |   4 +
 mm/xpfo.c                                       | 222 ++++++++++++++++++++++++
 security/Kconfig                                |  19 ++
 13 files changed, 449 insertions(+), 21 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +0 -0

index d9c171ce4190..444d83183f75 100644

Quoted 11 lines

Documentation/admin-guide/kernel-parameters.txt +2 -0

@@ -2736,6 +2736,8 @@
 
 	nox2apic	[X86-64,APIC] Do not enable x2APIC mode.
 
+	noxpfo		[X86-64] Disable XPFO when CONFIG_XPFO is on.
+
 	cpu0_hotplug	[X86] Turn on CPU0 hotplug feature when
 			CONFIG_BOOTPARAM_HOTPLUG_CPU0 is off.
 			Some features depend on CPU0. Known dependencies

<... snip>

A bit more description for system administrators would be very useful. Perhaps something like:

noxpfo [XPFO,X86-64] Disable eXclusive Page Frame Ownership (XPFO)
Physical pages mapped into user applications will also be mapped in the kernel's address space as if CONFIG_XPFO was not enabled.

Patch 05 should also update kernel-parameters.txt and add "ARM64" to the config option list for noxpfo.

arch/x86/include/asm/pgtable.h +5 -0

+/*
+ * The current flushing context - we pass it instead of 5 arguments:
+ */
+struct cpa_data {
+	unsigned long	*vaddr;

include/linux/highmem.h +7 -1

+#include <linux/xpfo.h>
 
 #include <asm/cacheflush.h>

@@ -55,24 +56,34 @@ static inline struct page *kmap_to_page(void *addr)
 #ifndef ARCH_HAS_KMAP
 static inline void *kmap(struct page *page)
 {
+	void *kaddr;
+
 	might_sleep();
-	return page_address(page);
+	kaddr = page_address(page);
+	xpfo_kmap(kaddr, page);
+	return kaddr;
 }
 
 static inline void kunmap(struct page *page)
 {
+	xpfo_kunmap(page_address(page), page);
 }
 
 static inline void *kmap_atomic(struct page *page)

Quoted 23 lines

include/linux/highmem.h +7 -1

+#include <linux/xpfo.h>
 
 #include <asm/cacheflush.h>

@@ -55,24 +56,34 @@ static inline struct page *kmap_to_page(void *addr)
 #ifndef ARCH_HAS_KMAP
 static inline void *kmap(struct page *page)
 {
+	void *kaddr;
+
 	might_sleep();
-	return page_address(page);
+	kaddr = page_address(page);
+	xpfo_kmap(kaddr, page);
+	return kaddr;
 }
 
 static inline void kunmap(struct page *page)
 {
+	xpfo_kunmap(page_address(page), page);
 }
 
 static inline void *kmap_atomic(struct page *page)

include/linux/highmem.h static inline struct page *kmap_to_page(void *addr) · line 81

Quoted 10 lines

include/linux/highmem.h +5 -1

 {
+	void *kaddr;
+
 	preempt_disable();
 	pagefault_disable();
-	return page_address(page);
+	kaddr = page_address(page);
+	xpfo_kmap(kaddr, page);
+	return kaddr;
 }

It seems to me like we should simply direct to pure xpfo implementations for the !HIGHMEM && XPFO case. - that is just have the prototypes for kmap, kunmap and co in linux/highmem.h and implement them in xpfo under those names.

Instead of sprinkling them around.

Quoted 9 lines · Tycho Andersen

mm/xpfo.c new file +9 -0

+static inline struct xpfo *lookup_xpfo(struct page *page)
+{
+	struct page_ext *page_ext = lookup_page_ext(page);
+
+	if (unlikely(!page_ext)) {
+		WARN(1, "xpfo: failed to get page ext");
+		return NULL;
+	}
+

mm/xpfo.c line 83

Quoted 3 lines

mm/xpfo.c new file +3 -0

+	return (void *)page_ext + page_xpfo_ops.offset;
+}
+

Just drop the WARN. On my arm64 UEFI machine this spews warnings under most normal operation. This should be normal for some situations but I haven't had the time to dig into why this is so pronounced on arm64.

From: Juerg Haefliger <[email protected]>

This patch adds support for XPFO which protects against 'ret2dir' kernel attacks. The basic idea is to enforce exclusive ownership of page frames by either the kernel or userspace, unless explicitly requested by the kernel. Whenever a page destined for userspace is allocated, it is unmapped from physmap (the kernel's page table). When such a page is reclaimed from userspace, it is mapped back to physmap.

Additional fields in the page_ext struct are used for XPFO housekeeping, specifically:
- two flags to distinguish user vs. kernel pages and to tag unmapped pages.
- a reference counter to balance kmap/kunmap operations.
- a lock to serialize access to the XPFO fields.

This patch is based on the work of Vasileios P. Kemerlis et al. who published their work in this paper:
http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf

[...]

mm/xpfo.c new file +22 -0

+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+{
+	int i, flush_tlb = 0;
+	struct xpfo *xpfo;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	for (i = 0; i < (1 << order); i++)  {
+		xpfo = lookup_xpfo(page + i);
+		if (!xpfo)
+			continue;
+
+		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
+		     "xpfo: unmapped page being allocated\n");
+
+		/* Initialize the map lock and map counter */
+		if (unlikely(!xpfo->inited)) {
+			spin_lock_init(&xpfo->maplock);
+			atomic_set(&xpfo->mapcount, 0);
+			xpfo->inited = true;
+		}

Quoted 28 lines

mm/xpfo.c new file +28 -0

+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+{
+	int i, flush_tlb = 0;
+	struct xpfo *xpfo;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	for (i = 0; i < (1 << order); i++)  {
+		xpfo = lookup_xpfo(page + i);
+		if (!xpfo)
+			continue;
+
+		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
+		     "xpfo: unmapped page being allocated\n");
+
+		/* Initialize the map lock and map counter */
+		if (unlikely(!xpfo->inited)) {
+			spin_lock_init(&xpfo->maplock);
+			atomic_set(&xpfo->mapcount, 0);
+			xpfo->inited = true;
+		}
+		WARN(atomic_read(&xpfo->mapcount),
+		     "xpfo: already mapped page being allocated\n");
+
+		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
+			/*
+			 * Tag the page as a user page and flush the TLB if it

mm/xpfo.c line 115

Quoted 4 lines

mm/xpfo.c new file +4 -0

+			 * was previously allocated to the kernel.
+			 */
+			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
+				flush_tlb = 1;

I'm not sure whether I am miss anything, however, when the page was previously allocated to kernel, should we unmap the physmap (the kernel's page table) here? For we allocate the page to user now

Hi Yisheng,

Quoted 28 lines · Yisheng Xie

Quoted 28 lines

mm/xpfo.c new file +28 -0

+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+{
+	int i, flush_tlb = 0;
+	struct xpfo *xpfo;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	for (i = 0; i < (1 << order); i++)  {
+		xpfo = lookup_xpfo(page + i);
+		if (!xpfo)
+			continue;
+
+		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
+		     "xpfo: unmapped page being allocated\n");
+
+		/* Initialize the map lock and map counter */
+		if (unlikely(!xpfo->inited)) {
+			spin_lock_init(&xpfo->maplock);
+			atomic_set(&xpfo->mapcount, 0);
+			xpfo->inited = true;
+		}
+		WARN(atomic_read(&xpfo->mapcount),
+		     "xpfo: already mapped page being allocated\n");
+
+		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
+			/*
+			 * Tag the page as a user page and flush the TLB if it

Quoted 9 lines

mm/xpfo.c line 115

Quoted 4 lines

mm/xpfo.c new file +4 -0

+			 * was previously allocated to the kernel.
+			 */
+			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
+				flush_tlb = 1;

I'm not sure whether I am miss anything, however, when the page was previously allocated to kernel, should we unmap the physmap (the kernel's page table) here? For we allocate the page to user now

Yes, I think you're right. Oddly, the XPFO_READ_USER test works correctly for me, but I think (?) should not because of this bug...

Tycho

Quoted 40 lines · Tycho Andersen

Hi Yisheng,

Quoted 37 lines · Yisheng Xie

Quoted 22 lines

mm/xpfo.c new file +22 -0

+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+{
+	int i, flush_tlb = 0;
+	struct xpfo *xpfo;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	for (i = 0; i < (1 << order); i++)  {
+		xpfo = lookup_xpfo(page + i);
+		if (!xpfo)
+			continue;
+
+		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
+		     "xpfo: unmapped page being allocated\n");
+
+		/* Initialize the map lock and map counter */
+		if (unlikely(!xpfo->inited)) {
+			spin_lock_init(&xpfo->maplock);
+			atomic_set(&xpfo->mapcount, 0);
+			xpfo->inited = true;
+		}

mm/xpfo.c line 115

Quoted 10 lines

mm/xpfo.c new file +10 -0

+		WARN(atomic_read(&xpfo->mapcount),
+		     "xpfo: already mapped page being allocated\n");
+
+		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
+			/*
+			 * Tag the page as a user page and flush the TLB if it
+			 * was previously allocated to the kernel.
+			 */
+			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
+				flush_tlb = 1;

I'm not sure whether I am miss anything, however, when the page was previously allocated to kernel, should we unmap the physmap (the kernel's page table) here? For we allocate the page to user now

Quoted 2 lines

Yes, I think you're right. Oddly, the XPFO_READ_USER test works correctly for me, but I think (?) should not because of this bug...

IIRC, this is an optimization carried forward from the initial implementation. The assumption is that the kernel will map the user buffer so it's not unmapped on allocation but only on the first (and

subsequent) call of kunmap. I.e.:
- alloc -> noop
- kmap -> noop
- kunmap -> unmapped from the kernel
- kmap -> mapped into the kernel
- kunmap -> unmapped from the kernel
and so on until:
- free -> mapped back into the kernel

I'm not sure if that make sense though since it leaves a window.

...Juerg

Quoted 2 lines

Tycho

Quoted 40 lines · Tycho Andersen

Hi Yisheng,

Quoted 37 lines · Yisheng Xie

Quoted 22 lines

mm/xpfo.c new file +22 -0

+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+{
+	int i, flush_tlb = 0;
+	struct xpfo *xpfo;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	for (i = 0; i < (1 << order); i++)  {
+		xpfo = lookup_xpfo(page + i);
+		if (!xpfo)
+			continue;
+
+		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
+		     "xpfo: unmapped page being allocated\n");
+
+		/* Initialize the map lock and map counter */
+		if (unlikely(!xpfo->inited)) {
+			spin_lock_init(&xpfo->maplock);
+			atomic_set(&xpfo->mapcount, 0);
+			xpfo->inited = true;
+		}

mm/xpfo.c line 115

Quoted 10 lines

mm/xpfo.c new file +10 -0

+		WARN(atomic_read(&xpfo->mapcount),
+		     "xpfo: already mapped page being allocated\n");
+
+		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
+			/*
+			 * Tag the page as a user page and flush the TLB if it
+			 * was previously allocated to the kernel.
+			 */
+			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
+				flush_tlb = 1;

I'm not sure whether I am miss anything, however, when the page was previously allocated to kernel, should we unmap the physmap (the kernel's page table) here? For we allocate the page to user now

Quoted 1 line

Yes, I think you're right. Oddly, the XPFO_READ_USER test works

Quoted 1 line

correctly for me, but I think (?) should not because of this bug...

IIRC, this is an optimization carried forward from the initial implementation.

The assumption is that the kernel will map the user buffer so it's not unmapped on allocation but only on the first (and subsequent) call of kunmap.

I.e.:
- alloc -> noop
- kmap -> noop
- kunmap -> unmapped from the kernel
- kmap -> mapped into the kernel
- kunmap -> unmapped from the kernel
and so on until:
- free -> mapped back into the kernel

I'm not sure if that make sense though since it leaves a window.

...Juerg

Quoted 2 lines

Tycho

.

Quoted 43 lines · Juerg Haefliger

Quoted 40 lines · Tycho Andersen

Hi Yisheng,

Quoted 37 lines · Yisheng Xie

Quoted 22 lines

mm/xpfo.c new file +22 -0

+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+{
+	int i, flush_tlb = 0;
+	struct xpfo *xpfo;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	for (i = 0; i < (1 << order); i++)  {
+		xpfo = lookup_xpfo(page + i);
+		if (!xpfo)
+			continue;
+
+		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
+		     "xpfo: unmapped page being allocated\n");
+
+		/* Initialize the map lock and map counter */
+		if (unlikely(!xpfo->inited)) {
+			spin_lock_init(&xpfo->maplock);
+			atomic_set(&xpfo->mapcount, 0);
+			xpfo->inited = true;
+		}

mm/xpfo.c line 115

Quoted 10 lines

mm/xpfo.c new file +10 -0

+		WARN(atomic_read(&xpfo->mapcount),
+		     "xpfo: already mapped page being allocated\n");
+
+		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
+			/*
+			 * Tag the page as a user page and flush the TLB if it
+			 * was previously allocated to the kernel.
+			 */
+			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
+				flush_tlb = 1;

I'm not sure whether I am miss anything, however, when the page was previously allocated to kernel, should we unmap the physmap (the kernel's page table) here? For we allocate the page to user now

Quoted 1 line

Quoted 1 line

Yes, I think you're right. Oddly, the XPFO_READ_USER test works

Hi Tycho,
Could you share this test? I'd like to know how it works.

Quoted 4 lines

Quoted 1 line

correctly for me, but I think (?) should not because of this bug...

IIRC, this is an optimization carried forward from the initial implementation.

Hi Juerg,

hmm.. If below is the first version, then it seems this exist from the first version:
https://patchwork.kernel.org/patch/8437451/

Quoted 3 lines

The assumption is that the kernel will map the user buffer so it's not unmapped on allocation but only on the first (and subsequent) call of kunmap.

IMO, before a page is allocated, it is in buddy system, which means it is free and no other 'map' on the page except direct map. Then if the page is allocated to user, XPFO should unmap the direct map. otherwise the ret2dir may works at this window before it is freed. Or maybe I'm still missing anything.

Quoted 4 lines

IMO, before a page is allocated, it is in buddy system, which means it is free and no other 'map' on the page except direct map. Then if the page is allocated to user, XPFO should unmap the direct map. otherwise the ret2dir may works at this window before it is freed. Or maybe I'm still missing anything.

I agree that it seems broken. I'm just not sure why the test doesn't fail. It's certainly worth understanding.

From bfc21a6438cf8c56741af94cac939f1b0f63752c Mon Sep 17 00:00:00 2001

Hi Yisheng,

Quoted 8 lines · Yisheng Xie

Quoted 4 lines

IMO, before a page is allocated, it is in buddy system, which means it is free and no other 'map' on the page except direct map. Then if the page is allocated to user, XPFO should unmap the direct map. otherwise the ret2dir may works at this window before it is freed. Or maybe I'm still missing anything.

I agree that it seems broken. I'm just not sure why the test doesn't fail. It's certainly worth understanding.

Ok, so I think what's going on is that the page *is* mapped and unmapped by the kernel as Juerg described, but only in certain cases. See prep_new_page(), which has the following:

	if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
		for (i = 0; i < (1 << order); i++)
			clear_highpage(page + i);

clear_highpage() maps and unmaps the pages, so that's why xpfo works with this set.

I tried with CONFIG_PAGE_POISONING_ZERO=y and page_poison=y, and the XPFO_READ_USER test does not fail, i.e. the read succeeds. So, I think we need to include this zeroing condition in xpfo_alloc_pages(), something like the patch below. Unfortunately, this fails to boot for me, probably for an unrelated reason that I'll look into.

Thanks a lot!

Tycho

Quoted 1 line

Quoted 1 line

From bfc21a6438cf8c56741af94cac939f1b0f63752c Mon Sep 17 00:00:00 2001

From: Tycho Andersen <[email protected]>
Date: Tue, 12 Sep 2017 12:06:41 -0600
Subject: [PATCH] draft of unmapping patch

Signed-off-by Tycho Andersen <[email protected]>

---
 include/linux/xpfo.h |  5 +++--
 mm/compaction.c      |  2 +-
 mm/internal.h        |  2 +-
 mm/page_alloc.c      | 10 ++++++----
 mm/xpfo.c            | 10 ++++++++--
 5 files changed, 19 insertions(+), 10 deletions(-)

include/linux/xpfo.h +3 -2

index b24be9ac4a2d..c991bf7f051d 100644
@@ -29,7 +29,7 @@ void xpfo_flush_kernel_tlb(struct page *page, int order);
 
 void xpfo_kmap(void *kaddr, struct page *page);
 void xpfo_kunmap(void *kaddr, struct page *page);
-void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp);
+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map);
 void xpfo_free_pages(struct page *page, int order);
 
 bool xpfo_page_is_unmapped(struct page *page);

@@ -49,7 +49,8 @@ void xpfo_temp_unmap(const void *addr, size_t size, void **mapping,
 
 static inline void xpfo_kmap(void *kaddr, struct page *page) { }
 static inline void xpfo_kunmap(void *kaddr, struct page *page) { }
-static inline void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp) { }
+static inline void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp,
+				    bool will_map) { }
 static inline void xpfo_free_pages(struct page *page, int order) { }
 
 static inline bool xpfo_page_is_unmapped(struct page *page) { return false; }

mm/compaction.c +1 -1

index fb548e4c7bd4..9a222258e65c 100644
@@ -76,7 +76,7 @@ static void map_pages(struct list_head *list)
 		order = page_private(page);
 		nr_pages = 1 << order;
 
-		post_alloc_hook(page, order, __GFP_MOVABLE);
+		post_alloc_hook(page, order, __GFP_MOVABLE, false);
 		if (order)
 			split_page(page, order);
 

mm/internal.h +1 -1

index 4ef49fc55e58..1a0331ec2b2d 100644
@@ -165,7 +165,7 @@ extern void __free_pages_bootmem(struct page *page, unsigned long pfn,
 					unsigned int order);
 extern void prep_compound_page(struct page *page, unsigned int order);
 extern void post_alloc_hook(struct page *page, unsigned int order,
-					gfp_t gfp_flags);
+					gfp_t gfp_flags, bool will_map);
 extern int user_min_free_kbytes;
 
 #if defined CONFIG_COMPACTION || defined CONFIG_CMA

mm/page_alloc.c +6 -4

index 09fdf1bad21f..f73809847c58 100644
@@ -1750,7 +1750,7 @@ static bool check_new_pages(struct page *page, unsigned int order)
 }
 
 inline void post_alloc_hook(struct page *page, unsigned int order,
-				gfp_t gfp_flags)
+				gfp_t gfp_flags, bool will_map)
 {
 	set_page_private(page, 0);
 	set_page_refcounted(page);

@@ -1759,18 +1759,20 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	kernel_map_pages(page, 1 << order, 1);
 	kernel_poison_pages(page, 1 << order, 1);
 	kasan_alloc_pages(page, order);
-	xpfo_alloc_pages(page, order, gfp_flags);
+	xpfo_alloc_pages(page, order, gfp_flags, will_map);
 	set_page_owner(page, order, gfp_flags);
 }
 
+extern bool xpfo_test;
 static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
 							unsigned int alloc_flags)
 {
 	int i;
+	bool needs_zero = !free_pages_prezeroed() && (gfp_flags & __GFP_ZERO);
 
-	post_alloc_hook(page, order, gfp_flags);
+	post_alloc_hook(page, order, gfp_flags, needs_zero);
 
-	if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
+	if (needs_zero)
 		for (i = 0; i < (1 << order); i++)
 			clear_highpage(page + i);
 

mm/xpfo.c +8 -2

index ca5d4d1838f9..dd25e24213fe 100644
@@ -86,7 +86,7 @@ static inline struct xpfo *lookup_xpfo(struct page *page)
 	return (void *)page_ext + page_xpfo_ops.offset;
 }
 
-void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map)
 {
 	int i, flush_tlb = 0;
 	struct xpfo *xpfo;

@@ -116,8 +116,14 @@ void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
 			 * Tag the page as a user page and flush the TLB if it
 			 * was previously allocated to the kernel.
 			 */
-			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
+			bool was_user = !test_and_set_bit(XPFO_PAGE_USER,
+							  &xpfo->flags);
+
+			if (was_user || !will_map) {
+				set_kpte(page_address(page + i), page + i,
+					 __pgprot(0));
 				flush_tlb = 1;
+			}
 		} else {
 			/* Tag the page as a non-user (kernel) page */
 			clear_bit(XPFO_PAGE_USER, &xpfo->flags);

mm/xpfo.c +1 -1

-void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map)
 {
 	int i, flush_tlb = 0;
 	struct xpfo *xpfo;

@@ -116,8 +116,14 @@ void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
 			 * Tag the page as a user page and flush the TLB if it
 			 * was previously allocated to the kernel.

Quoted 12 lines · Tycho Andersen

mm/xpfo.c +3 -2

-void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map)
 {
 	int i, flush_tlb = 0;
 	struct xpfo *xpfo;

@@ -116,8 +116,14 @@ void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
 			 * Tag the page as a user page and flush the TLB if it
 			 * was previously allocated to the kernel.
 			 */
-			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
+			bool was_user = !test_and_set_bit(XPFO_PAGE_USER,
+							  &xpfo->flags);

mm/xpfo.c void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp) · line 126

Quoted 6 lines

mm/xpfo.c +5 -0

+
+			if (was_user || !will_map) {
+				set_kpte(page_address(page + i), page + i,
+					 __pgprot(0));
 				flush_tlb = 1;
+			}

Shouldn't the "was_user" be "was_kernel"?

Also, the way this now works, let's say we have a nice, 2MB pmd_t (page table entry) mapping a nice, 2MB page in the allocator. Then it gets allocated to userspace. We do

	for (i = 0; i < (1 << order); i++)  {
		...
		set_kpte(page_address(page + i), page+i, __pgprot(0));
	}

The set_kpte() will take the nice, 2MB mapping and break it down into 512 4k mappings, all pointing to a non-present PTE, in a newly-allocated PTE page. So, you get the same result and waste 4k of memory in the process, *AND* make it slower because we added a level to the page tables.

I think you actually want to make a single set_kpte() call at the end of the function. That's faster and preserves the large page in the direct mapping.

...and makes it easier to pair tlb flushes with changing the protections. I guess we still need the for loop, because we need to set/unset the xpfo bits as necessary, but I'll switch it to a single set_kpte(). This also implies that the xpfo bits should all be the same on every page in the mapping, which I think is true.

arch/x86/mm/xpfo.c new file +18 -0

+inline void xpfo_flush_kernel_tlb(struct page *page, int order)
+{
+	int level;
+	unsigned long size, kaddr;
+
+	kaddr = (unsigned long)page_address(page);
+
+	if (unlikely(!lookup_address(kaddr, &level))) {
+		WARN(1, "xpfo: invalid address to flush %lx %d\n", kaddr, level);
+		return;
+	}
+
+	switch (level) {
+	case PG_LEVEL_4K:
+		size = PAGE_SIZE;
+		break;
+	case PG_LEVEL_2M:
+		size = PMD_SIZE;

Hi all,

Quoted 19 lines · Tycho Andersen

arch/x86/mm/xpfo.c new file +18 -0

+inline void xpfo_flush_kernel_tlb(struct page *page, int order)
+{
+     int level;
+     unsigned long size, kaddr;
+
+     kaddr = (unsigned long)page_address(page);
+
+     if (unlikely(!lookup_address(kaddr, &level))) {
+             WARN(1, "xpfo: invalid address to flush %lx %d\n", kaddr, level);
+             return;
+     }
+
+     switch (level) {
+     case PG_LEVEL_4K:
+             size = PAGE_SIZE;
+             break;
+     case PG_LEVEL_2M:
+             size = PMD_SIZE;

arch/x86/mm/xpfo.c line 113

Quoted 10 lines

arch/x86/mm/xpfo.c new file +10 -0

+             break;
+     case PG_LEVEL_1G:
+             size = PUD_SIZE;
+             break;
+     default:
+             WARN(1, "xpfo: unsupported page level %x\n", level);
+             return;
+     }
+
+     flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);

Marco was testing and got the stack trace below. The issue is that on x86, flush_tlb_kernel_range uses on_each_cpu, which causes the WARN() below. Since this is called from xpfo_kmap/unmap in this interrupt handler, the WARN() triggers.

I'm not sure what to do about this -- based on the discussion in v6 we need to flush the TLBs for all CPUs -- but we can't do that with interrupts disabled, which basically means with this we wouldn't be able to map/unmap pages in interrupts.

Any thoughts?

Tycho

[    2.712912] ------------[ cut here ]------------
[    2.712922] WARNING: CPU: 0 PID: 0 at kernel/smp.c:414
smp_call_function_many+0x9a/0x270
[    2.712923] Modules linked in: sd_mod ata_generic pata_acpi qxl

drm_kms_helper syscopyarea sysfillrect virtio_console sysimgblt virtio_blk fb_sys_fops ttm drm 8139too ata_piix libata 8139cp virtio_pci virtio_ring virtio mii crc32c_intel i2c_core serio_raw floppy dm_mirror dm_region_hash dm_log dm_mod

[    2.712939] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.13.0+ #8
[    2.712940] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.9.3-1.fc25 04/01/2014
[    2.712941] task: ffffffff81c10480 task.stack: ffffffff81c00000
[    2.712943] RIP: 0010:smp_call_function_many+0x9a/0x270
[    2.712944] RSP: 0018:ffff88023fc03b38 EFLAGS: 00010046
[    2.712945] RAX: 0000000000000000 RBX: ffffffff81072a50 RCX: 0000000000000001
[    2.712946] RDX: ffff88023fc03ba8 RSI: ffffffff81072a50 RDI: ffffffff81e22320
[    2.712947] RBP: ffff88023fc03b70 R08: 0000000000000970 R09: 0000000000000063
[    2.712948] R10: ffff880000000970 R11: 0000000000000000 R12: ffff88023fc03ba8
[    2.712949] R13: 0000000000000000 R14: ffff8802332b8e18 R15: ffffffff81e22320
[    2.712950] FS:  0000000000000000(0000) GS:ffff88023fc00000(0000)
knlGS:0000000000000000
[    2.712951] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    2.712951] CR2: 00007fde22f6b000 CR3: 000000022727b000 CR4: 00000000003406f0
[    2.712954] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    2.712955] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[    2.712955] Call Trace:
[    2.712959]  <IRQ>
[    2.712964]  ? x86_configure_nx+0x50/0x50
[    2.712966]  on_each_cpu+0x2d/0x60
[    2.712967]  flush_tlb_kernel_range+0x79/0x80
[    2.712969]  xpfo_flush_kernel_tlb+0xaa/0xe0
[    2.712975]  xpfo_kunmap+0xa8/0xc0
[    2.712981]  swiotlb_bounce+0xd1/0x1c0
[    2.712982]  swiotlb_tbl_unmap_single+0x10f/0x120
[    2.712984]  unmap_single+0x20/0x30
[    2.712985]  swiotlb_unmap_sg_attrs+0x46/0x70
[    2.712991]  __ata_qc_complete+0xfa/0x150 [libata]
[    2.712994]  ata_qc_complete+0xd2/0x2e0 [libata]
[    2.712998]  ata_hsm_qc_complete+0x6f/0x90 [libata]
[    2.713004]  ata_sff_hsm_move+0xae/0x6b0 [libata]
[    2.713009]  __ata_sff_port_intr+0x8e/0x100 [libata]
[    2.713013]  ata_bmdma_port_intr+0x2f/0xd0 [libata]
[    2.713019]  ata_bmdma_interrupt+0x161/0x1b0 [libata]
[    2.713022]  __handle_irq_event_percpu+0x3c/0x190
[    2.713024]  handle_irq_event_percpu+0x32/0x80
[    2.713026]  handle_irq_event+0x3b/0x60
[    2.713027]  handle_edge_irq+0x8f/0x190
[    2.713029]  handle_irq+0xab/0x120
[    2.713032]  ? _local_bh_enable+0x21/0x30
[    2.713039]  do_IRQ+0x48/0xd0
[    2.713040]  common_interrupt+0x93/0x93
[    2.713042] RIP: 0010:native_safe_halt+0x6/0x10
[    2.713043] RSP: 0018:ffffffff81c03de0 EFLAGS: 00000246 ORIG_RAX:
ffffffffffffffc1
[    2.713044] RAX: 0000000000000000 RBX: ffffffff81c10480 RCX: 0000000000000000
[    2.713045] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
[    2.713046] RBP: ffffffff81c03de0 R08: 00000000b656100b R09: 0000000000000000
[    2.713047] R10: 0000000000000006 R11: 0000000000000005 R12: 0000000000000000
[    2.713047] R13: ffffffff81c10480 R14: 0000000000000000 R15: 0000000000000000
[    2.713048]  </IRQ>
[    2.713050]  default_idle+0x1e/0x100
[    2.713052]  arch_cpu_idle+0xf/0x20
[    2.713053]  default_idle_call+0x2c/0x40
[    2.713055]  do_idle+0x158/0x1e0
[    2.713056]  cpu_startup_entry+0x73/0x80
[    2.713058]  rest_init+0xb8/0xc0
[    2.713070]  start_kernel+0x4a2/0x4c3
[    2.713072]  ? set_init_arg+0x5a/0x5a
[    2.713074]  ? early_idt_handler_array+0x120/0x120
[    2.713075]  x86_64_start_reservations+0x2a/0x2c
[    2.713077]  x86_64_start_kernel+0x14c/0x16f
[    2.713079]  secondary_startup_64+0x9f/0x9f
[    2.713080] Code: 44 3b 35 1e 6f d0 00 7c 26 48 83 c4 10 5b 41 5c
41 5d 41 5e 41 5f 5d c3 8b 05 63 38 fc 00 85 c0 75 be 80 3d 20 0d d0
00 00 75 b5 <0f> ff eb b1 48 c7 c2 20 23 e2 81 4c 89 fe 44 89 f7 e8 20
b5 62
[    2.713105] ---[ end trace 4d101d4c176c16b0 ]---

Whenever a page destined for userspace is allocated, it is unmapped from physmap (the kernel's page table). When such a page is reclaimed from userspace, it is mapped back to physmap.

arch/x86/mm/xpfo.c new file +19 -0

+inline void xpfo_flush_kernel_tlb(struct page *page, int order)
+{
+	int level;
+	unsigned long size, kaddr;
+
+	kaddr = (unsigned long)page_address(page);
+
+	if (unlikely(!lookup_address(kaddr, &level))) {
+		WARN(1, "xpfo: invalid address to flush %lx %d\n", kaddr, level);
+		return;
+	}
+
+	switch (level) {
+	case PG_LEVEL_4K:
+		size = PAGE_SIZE;
+		break;
+	case PG_LEVEL_2M:
+		size = PMD_SIZE;
+		break;

mm/xpfo.c new file +3 -0

+
+void xpfo_free_pages(struct page *page, int order)
+{

On 09/07/2017 10:36 AM, Tycho Andersen wrote:
...

Quoted 3 lines

Whenever a page destined for userspace is allocated, it is unmapped from physmap (the kernel's page table). When such a page is reclaimed from userspace, it is mapped back to physmap.

I'm looking for the code where it's unmapped at allocation to userspace.
I see TLB flushing and 'struct xpfo' manipulation, but I don't see the
unmapping. Where is that occurring?

How badly does this hurt performance? Since we (generally) have different migrate types for user and kernel allocation, I can imagine that a given page *generally* doesn't oscillate between user and kernel-allocated, but I'm curious how it works in practice. Doesn't the IPI load from the TLB flushes eat you alive?

It's a bit scary to have such a deep code path under the main allocator.

This all seems insanely expensive. It will *barely* work on an allocation-heavy workload on a desktop. I'm pretty sure the locking will just fall over entirely on any reasonably-sized server.

I really have to wonder whether there are better ret2dir defenses than this. The allocator just seems like the *wrong* place to be doing this because it's such a hot path.

Quoted 8 lines

arch/x86/mm/xpfo.c new file +8 -0

+		cpa.vaddr = kaddr;
+		cpa.pages = &page;
+		cpa.mask_set = prot;
+		cpa.mask_clr = msk_clr;
+		cpa.numpages = 1;
+		cpa.flags = 0;
+		cpa.curpage = 0;
+		cpa.force_split = 0;

arch/x86/include/asm/pgtable.h static inline bool pud_access_permitted(pud_t pud, bool write)

Quoted 3 lines

arch/x86/include/asm/pgtable.h +3 -0

+
+
+		do_split = try_preserve_large_page(pte, (unsigned 

Is this safe to do without a TLB flush? I thought we had plenty of bugs in CPUs around having multiple entries for the same page in the TLB at once. We're *REALLY* careful when we split large pages for THP, and I'm surprised we don't do the same here.

Why do you even bother keeping large pages around? Won't the entire kernel just degrade to using 4k everywhere, eventually?

arch/x86/mm/xpfo.c

Quoted 4 lines

arch/x86/mm/xpfo.c new file +4 -0

+		if (do_split) {
+			struct page *base;
+
+			base = alloc_pages(GFP_ATOMIC | __GFP_NOTRACK, 

Ugh, GFP_ATOMIC. That's nasty. Do you really want this allocation to fail all the time? GFP_ATOMIC could really be called GFP_YOU_BETTER_BE_OK_WITH_THIS_FAILING. :)

You probably want to do what the THP code does here and keep a spare page around, then allocate it before you take the locks.

Quoted 19 lines

arch/x86/mm/xpfo.c new file +19 -0

+inline void xpfo_flush_kernel_tlb(struct page *page, int order)
+{
+	int level;
+	unsigned long size, kaddr;
+
+	kaddr = (unsigned long)page_address(page);
+
+	if (unlikely(!lookup_address(kaddr, &level))) {
+		WARN(1, "xpfo: invalid address to flush %lx %d\n", kaddr, level);
+		return;
+	}
+
+	switch (level) {
+	case PG_LEVEL_4K:
+		size = PAGE_SIZE;
+		break;
+	case PG_LEVEL_2M:
+		size = PMD_SIZE;
+		break;

arch/x86/mm/xpfo.c line 114

Quoted 10 lines

arch/x86/mm/xpfo.c new file +10 -0

+	case PG_LEVEL_1G:
+		size = PUD_SIZE;
+		break;
+	default:
+		WARN(1, "xpfo: unsupported page level %x\n", level);
+		return;
+	}
+
+	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);
+}

I'm not sure flush_tlb_kernel_range() is the best primitive to be calling here.

Let's say you walk the page tables and find level=PG_LEVEL_1G. You call flush_tlb_kernel_range(), you will be above tlb_single_page_flush_ceiling, and you will do a full TLB flush. But, with a 1GB page, you could have just used a single INVLPG and skipped the global flush.

I guess the cost of the IPI is way more than the flush itself, but it's still a shame to toss the entire TLB when you don't have to.

I also think the TLB flush should be done closer to the page table manipulation that it is connected to. It's hard to figure out whether the flush is the right one otherwise.

Also, the "(1 << order) * size" thing looks goofy to me. Let's say you are flushing a order=1 (8k) page and its mapped in a 1GB mapping. You flush 2GB. Is that intentional?

Quoted 3 lines

mm/xpfo.c new file +3 -0

+
+void xpfo_free_pages(struct page *page, int order)
+{

...

mm/xpfo.c line 154

Quoted 11 lines

mm/xpfo.c new file +11 -0

+		/*
+		 * Map the page back into the kernel if it was previously
+		 * allocated to user space.
+		 */
+		if (test_and_clear_bit(XPFO_PAGE_USER, &xpfo->flags)) {
+			clear_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags);
+			set_kpte(page_address(page + i), page + i,
+				 PAGE_KERNEL);
+		}
+	}
+}

This seems like a bad idea, performance-wise. Kernel and userspace pages tend to be separated by migrate types. So, a given physical page will tend to be used as kernel *or* for userspace. With this nugget, every time a userspace page is freed, we will go to the trouble of making it *back* into a kernel page. Then, when it is allocated again (probably as userspace), we will re-make it into a userspace page. That seems horribly inefficient.

Also, this weakens the security guarantees. Let's say you're mounting a ret2dir attack. You populate a page with your evil data and you know the kernel address for the page. All you have to do is coordinate your attack with freeing the page. You can control when it gets freed. Now, the xpfo_free_pages() helpfully just mapped your attack code back into the kernel.

Why not *just* do these moves at allocation time?

Quoted 3 lines

I really have to wonder whether there are better ret2dir defenses than this. The allocator just seems like the *wrong* place to be doing this because it's such a hot path.

This might be crazy, but what if we defer flushing of the kernel ranges until just before we return to userspace? We'd still manipulate the prot/xpfo bits for the pages, but then just keep a list of which ranges need to be flushed, and do the right thing before we return. This leaves a little window between the actual allocation and the flush, but userspace would need another thread in its threadgroup to predict the next allocation, write the bad stuff there, and do the exploit all in that window.

Quoted 2 lines

Why do you even bother keeping large pages around? Won't the entire kernel just degrade to using 4k everywhere, eventually?

Isn't that true of large pages in general? Is there something about xpfo that makes this worse? I thought this would only split things if they had already been split somewhere else, and the protection can't apply to the whole huge page.

Quoted 39 lines

Quoted 19 lines

arch/x86/mm/xpfo.c new file +19 -0

+inline void xpfo_flush_kernel_tlb(struct page *page, int order)
+{
+	int level;
+	unsigned long size, kaddr;
+
+	kaddr = (unsigned long)page_address(page);
+
+	if (unlikely(!lookup_address(kaddr, &level))) {
+		WARN(1, "xpfo: invalid address to flush %lx %d\n", kaddr, level);
+		return;
+	}
+
+	switch (level) {
+	case PG_LEVEL_4K:
+		size = PAGE_SIZE;
+		break;
+	case PG_LEVEL_2M:
+		size = PMD_SIZE;
+		break;

arch/x86/mm/xpfo.c line 114

Quoted 10 lines

arch/x86/mm/xpfo.c new file +10 -0

+	case PG_LEVEL_1G:
+		size = PUD_SIZE;
+		break;
+	default:
+		WARN(1, "xpfo: unsupported page level %x\n", level);
+		return;
+	}
+
+	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);
+}

I'm not sure flush_tlb_kernel_range() is the best primitive to be calling here.

Let's say you walk the page tables and find level=PG_LEVEL_1G. You call flush_tlb_kernel_range(), you will be above tlb_single_page_flush_ceiling, and you will do a full TLB flush. But, with a 1GB page, you could have just used a single INVLPG and skipped the global flush.

Quoted 2 lines

I guess the cost of the IPI is way more than the flush itself, but it's still a shame to toss the entire TLB when you don't have to.

Ok, do you think it's worth making a new helper for others to use? Or should I just keep the logic in this function?

Quoted 4 lines · Tycho Andersen

Quoted 3 lines

I really have to wonder whether there are better ret2dir defenses than this. The allocator just seems like the *wrong* place to be doing this because it's such a hot path.

Quoted 8 lines

This might be crazy, but what if we defer flushing of the kernel ranges until just before we return to userspace? We'd still manipulate the prot/xpfo bits for the pages, but then just keep a list of which ranges need to be flushed, and do the right thing before we return. This leaves a little window between the actual allocation and the flush, but userspace would need another thread in its threadgroup to predict the next allocation, write the bad stuff there, and do the exploit all in that window.

I think the common case is still that you enter the kernel, allocate a single page (or very few) and then exit. So, you don't really reduce the total number of flushes.

Just think of this in terms of IPIs to do the remote TLB flushes. A CPU can do roughly 1 million page faults and allocations a second. Say you have a 2-socket x 28-core x 2 hyperthead system = 112 CPU threads. That's 111M IPI interrupts/second, just for the TLB flushes, *ON* *EACH* *CPU*.

I think the only thing that will really help here is if you batch the allocations. For instance, you could make sure that the per-cpu-pageset lists always contain either all kernel or all user data. Then remap the entire list at once and do a single flush after the entire list is consumed.

Quoted 4 lines

I think the only thing that will really help here is if you batch the allocations. For instance, you could make sure that the per-cpu-pageset lists always contain either all kernel or all user data. Then remap the entire list at once and do a single flush after the entire list is consumed.

Just so I understand, the idea would be that we only flush when the type of allocation alternates, so:

kmalloc(..., GFP_KERNEL);
kmalloc(..., GFP_KERNEL);
/* remap+flush here */
kmalloc(..., GFP_HIGHUSER);
/* remap+flush here */
kmalloc(..., GFP_KERNEL);

struct per_cpu_pages {

        int count;              /* number of pages in the list */
        int high;               /* high watermark, emptying needed */
        int batch;              /* chunk size for buddy add/remove */

        /* Lists of pages, one per migrate type stored on the pcp-lists */
        struct list_head lists[MIGRATE_PCPTYPES];
};

Quoted 7 lines · Tycho Andersen

Quoted 4 lines

I think the only thing that will really help here is if you batch the allocations. For instance, you could make sure that the per-cpu-pageset lists always contain either all kernel or all user data. Then remap the entire list at once and do a single flush after the entire list is consumed.

Just so I understand, the idea would be that we only flush when the type of allocation alternates, so:

Quoted 6 lines

kmalloc(..., GFP_KERNEL);
kmalloc(..., GFP_KERNEL);
/* remap+flush here */
kmalloc(..., GFP_HIGHUSER);
/* remap+flush here */
kmalloc(..., GFP_KERNEL);

Not really. We keep a free list per migrate type, and a per_cpu_pages (pcp) list per migratetype:

Quoted 8 lines

struct per_cpu_pages {

        int count;              /* number of pages in the list */
        int high;               /* high watermark, emptying needed */
        int batch;              /* chunk size for buddy add/remove */

        /* Lists of pages, one per migrate type stored on the pcp-lists */
        struct list_head lists[MIGRATE_PCPTYPES];
};

The migratetype is derived from the GFP flags in gfpflags_to_migratetype(). In general, GFP_HIGHUSER and GFP_KERNEL come from different migratetypes, so they come from different free lists.

In your case above, the GFP_HIGHUSER allocation come through the MIGRATE_MOVABLE pcp list while the GFP_KERNEL ones come from the MIGRATE_UNMOVABLE one. Since we add a bunch of pages to those lists at once, you could do all the mapping/unmapping/flushing on a bunch of pages at once

Or, you could hook your code into the places where the migratetype of memory is changed (set_pageblock_migratetype(), plus where we fall back). Those changes are much more rare than page allocation.

Quoted 7 lines · Tycho Andersen

Quoted 4 lines

I think the only thing that will really help here is if you batch the allocations. For instance, you could make sure that the per-cpu-pageset lists always contain either all kernel or all user data. Then remap the entire list at once and do a single flush after the entire list is consumed.

Just so I understand, the idea would be that we only flush when the type of allocation alternates, so:

Quoted 6 lines

kmalloc(..., GFP_KERNEL);
kmalloc(..., GFP_KERNEL);
/* remap+flush here */
kmalloc(..., GFP_HIGHUSER);
/* remap+flush here */
kmalloc(..., GFP_KERNEL);

Not really. We keep a free list per migrate type, and a per_cpu_pages (pcp) list per migratetype:

Quoted 8 lines

struct per_cpu_pages {

        int count;              /* number of pages in the list */
        int high;               /* high watermark, emptying needed */
        int batch;              /* chunk size for buddy add/remove */

        /* Lists of pages, one per migrate type stored on the pcp-lists */
        struct list_head lists[MIGRATE_PCPTYPES];
};

The migratetype is derived from the GFP flags in gfpflags_to_migratetype(). In general, GFP_HIGHUSER and GFP_KERNEL come from different migratetypes, so they come from different free lists.

In your case above, the GFP_HIGHUSER allocation come through the MIGRATE_MOVABLE pcp list while the GFP_KERNEL ones come from the MIGRATE_UNMOVABLE one. Since we add a bunch of pages to those lists at once, you could do all the mapping/unmapping/flushing on a bunch of pages at once

Or, you could hook your code into the places where the migratetype of memory is changed (set_pageblock_migratetype(), plus where we fall back). Those changes are much more rare than page allocation.

which I guess is from the additional flags in grow_dev_page() somewhere down the stack. Anyway... it seems this is a kernel allocation that's using MIGRATE_MOVABLE, so perhaps we need some more fine tuned heuristic than just all MOVABLE allocations are un-mapped via xpfo, and all the others are mapped.

Do you have any ideas?

Quoted 6 lines · Tycho Andersen

which I guess is from the additional flags in grow_dev_page() somewhere down the stack. Anyway... it seems this is a kernel allocation that's using MIGRATE_MOVABLE, so perhaps we need some more fine tuned heuristic than just all MOVABLE allocations are un-mapped via xpfo, and all the others are mapped.

Do you have any ideas?

It still has to do a kmap() or kmap_atomic() to be able to access it. I thought you hooked into that. Why isn't that path getting hit for these?

Quoted 10 lines · Dave Hansen

Quoted 6 lines · Tycho Andersen

which I guess is from the additional flags in grow_dev_page() somewhere down the stack. Anyway... it seems this is a kernel allocation that's using MIGRATE_MOVABLE, so perhaps we need some more fine tuned heuristic than just all MOVABLE allocations are un-mapped via xpfo, and all the others are mapped.

Do you have any ideas?

It still has to do a kmap() or kmap_atomic() to be able to access it. I thought you hooked into that. Why isn't that path getting hit for these?

Oh, this looks to be accessing data mapped by a buffer_head. It (rudely) accesses data via:

void set_bh_page(struct buffer_head *bh,
...

	bh->b_data = page_address(page) + offset;

Quoted 4 lines

void set_bh_page(struct buffer_head *bh,
...

	bh->b_data = page_address(page) + offset;

Ah, yes. I guess there will be many bugs like this :). Anyway, I'll try to cook up a patch.

Quoted 4 lines · Tycho Andersen

Quoted 4 lines

void set_bh_page(struct buffer_head *bh,
...

	bh->b_data = page_address(page) + offset;

Quoted 2 lines

Ah, yes. I guess there will be many bugs like this :). Anyway, I'll try to cook up a patch.

It won't catch all the bugs, but it might be handy to have a debugging mode that records the location of the last user of page_address() and friends. That way, when we trip over an unmapped page, we have an easier time finding the offender.

Quoted 10 lines · Dave Hansen

Quoted 6 lines · Tycho Andersen

which I guess is from the additional flags in grow_dev_page() somewhere down the stack. Anyway... it seems this is a kernel allocation that's using MIGRATE_MOVABLE, so perhaps we need some more fine tuned heuristic than just all MOVABLE allocations are un-mapped via xpfo, and all the others are mapped.

Do you have any ideas?

It still has to do a kmap() or kmap_atomic() to be able to access it. I thought you hooked into that. Why isn't that path getting hit for these?

Oh, this looks to be accessing data mapped by a buffer_head. It (rudely) accesses data via:

void set_bh_page(struct buffer_head *bh,
...

	bh->b_data = page_address(page) + offset;

Quoted 15 lines · Dave Hansen

Quoted 10 lines · Dave Hansen

Quoted 6 lines · Tycho Andersen

which I guess is from the additional flags in grow_dev_page() somewhere down the stack. Anyway... it seems this is a kernel allocation that's using MIGRATE_MOVABLE, so perhaps we need some more fine tuned heuristic than just all MOVABLE allocations are un-mapped via xpfo, and all the others are mapped.

Do you have any ideas?

It still has to do a kmap() or kmap_atomic() to be able to access it. I thought you hooked into that. Why isn't that path getting hit for these?

Oh, this looks to be accessing data mapped by a buffer_head. It (rudely) accesses data via:

Quoted 3 lines

void set_bh_page(struct buffer_head *bh,
...

	bh->b_data = page_address(page) + offset;

We don't need to kmap in order to access MOVABLE allocations. kmap is only needed for HIGHMEM allocations. So there's nothing wrong with ext4 or set_bh_page().

Quoted 3 lines · Matthew Wilcox

We don't need to kmap in order to access MOVABLE allocations. kmap is only needed for HIGHMEM allocations. So there's nothing wrong with ext4 or set_bh_page().

Yeah, it's definitely not _buggy_.

Although, I do wonder what we should do about these for XPFO. Should we just stick a kmap() in there and comment it? What we really need is a mechanism to say "use this as a kernel page" and "stop using this as a kernel page". kmap() does that... kinda. It's not a perfect fit, but it's pretty close.

Quoted 5 lines · Dave Hansen

Quoted 3 lines · Matthew Wilcox

We don't need to kmap in order to access MOVABLE allocations. kmap is only needed for HIGHMEM allocations. So there's nothing wrong with ext4 or set_bh_page().

Quoted 7 lines

Yeah, it's definitely not _buggy_.

Although, I do wonder what we should do about these for XPFO. Should we just stick a kmap() in there and comment it? What we really need is a mechanism to say "use this as a kernel page" and "stop using this as a kernel page". kmap() does that... kinda. It's not a perfect fit, but it's pretty close.

It'd be kind of funny if getting XPFO working better means improving how well Linux runs on 32-bit machines with HIGHMEM. I think there's always going to be interest in those -- ARM developed 36 bit physmem before biting the bullet and going to arm64. Maybe OpenRISC will do that next ;-)

Quoted 14 lines · Matthew Wilcox

Quoted 5 lines · Dave Hansen

Quoted 3 lines · Matthew Wilcox

We don't need to kmap in order to access MOVABLE allocations. kmap is only needed for HIGHMEM allocations. So there's nothing wrong with ext4 or set_bh_page().

Quoted 7 lines

Yeah, it's definitely not _buggy_.

Although, I do wonder what we should do about these for XPFO. Should we just stick a kmap() in there and comment it? What we really need is a mechanism to say "use this as a kernel page" and "stop using this as a kernel page". kmap() does that... kinda. It's not a perfect fit, but it's pretty close.

Quoted 5 lines

It'd be kind of funny if getting XPFO working better means improving how well Linux runs on 32-bit machines with HIGHMEM. I think there's always going to be interest in those -- ARM developed 36 bit physmem before biting the bullet and going to arm64. Maybe OpenRISC will do that next ;-)

Oh, sorry, I didn't realize that this wasn't a bug. In any case, this seems like sort of an uphill battle -- lots of places are going to do stuff like this since it's legal, adding code to work around it just for XPFO seems like a lot of burden on the kernel. (Of course, I'm open to convincing :)

How common are these MOVABLE allocations that the kernel does? What if we did some hybrid approach, where we re-map the lists based on MOVABLE/UNMOVABLE, but then check the actual GFP flags on allocation to see if they match what we set when populating the free list, and re-map accordingly if they don't.

At a high level, does this approach keep an attacker from being able to determine the address of data in the linear map, or does it keep them from being able to *exploit* it?

Can you have a ret2dir attack if the attacker doesn't know the address, for instance?

lib/swiotlb.c static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr, · line 425

Quoted 2 lines

lib/swiotlb.c +1 -1

-	if (PageHighMem(pfn_to_page(pfn))) {
+	if (PageHighMem(page) || xpfo_page_is_unmapped(page)) {

Please don't sprinkle xpfo details over various bits of code.

Just add a helper with a descriptive name, e.g.

page_is_unmapped()

that also includes the highmem case, as that will easily document what this check is doing.

Quoted 9 lines · Christoph Hellwig

lib/swiotlb.c static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr, · line 425

Quoted 2 lines

lib/swiotlb.c +1 -1

-	if (PageHighMem(pfn_to_page(pfn))) {
+	if (PageHighMem(page) || xpfo_page_is_unmapped(page)) {

Please don't sprinkle xpfo details over various bits of code.

Just add a helper with a descriptive name, e.g.

page_is_unmapped()

Quoted 2 lines

that also includes the highmem case, as that will easily document what this check is doing.

Will do, thanks.

Patch 7 has a similar feel to this one, I can add a wrapper around __clean_dcache_area_pou() if that makes sense?

Review

Quoted 5 lines

(quoted diff) +5 -0

+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */
+static pte_t *lookup_address(unsigned long addr)

Seems like this should be moved to common arm64 mm code and used by kernel_page_present.

From 0b3be95873e3e8caa39fa50efc0d06d57fc6eb5e Mon Sep 17 00:00:00 2001

Quoted 8 lines · Christoph Hellwig

Review

Quoted 5 lines

(quoted diff) +5 -0

+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */
+static pte_t *lookup_address(unsigned long addr)

Seems like this should be moved to common arm64 mm code and used by kernel_page_present.

Sounds good, I'll include something like the patch below in the next series.

Unfortunately, adding an implementation of lookup_address seems to be slightly more complicated than necessary, because of the xen piece. We have to define lookup_address() with the level parameter, but it's not obvious to me to name the page levels. So for now I've just left it as a WARN() if someone supplies it.

It seems like xen still does need this to be defined, because if I define it without level:

drivers/xen/xenbus/xenbus_client.c: In function ‘xenbus_unmap_ring_vfree_pv’:
drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to function ‘lookup_address’

     lookup_address(addr, &level)).maddr;
     ^~~~~~~~~~~~~~
In file included from ./arch/arm64/include/asm/page.h:37:0,
                  from ./include/linux/mmzone.h:20,
                  from ./include/linux/gfp.h:5,
                  from ./include/linux/mm.h:9,
                  from drivers/xen/xenbus/xenbus_client.c:33:
./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here
  extern pte_t *lookup_address(unsigned long addr);
                ^~~~~~~~~~~~~~

I've cc-d the xen folks, maybe they can suggest a way to untangle it? Alternatively, if someone can suggest a good naming scheme for the page levels, I can just do that.

Hi,

CC Juergen, Boris and Stefano.

Quoted 38 lines · Tycho Andersen

Quoted 9 lines · Christoph Hellwig

Quoted 6 lines

+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */
+static pte_t *lookup_address(unsigned long addr)

Seems like this should be moved to common arm64 mm code and used by kernel_page_present.

Sounds good, I'll include something like the patch below in the next series.

Unfortunately, adding an implementation of lookup_address seems to be slightly more complicated than necessary, because of the xen piece. We have to define lookup_address() with the level parameter, but it's not obvious to me to name the page levels. So for now I've just left it as a WARN() if someone supplies it.

It seems like xen still does need this to be defined, because if I define it without level:

drivers/xen/xenbus/xenbus_client.c: In function ‘xenbus_unmap_ring_vfree_pv’:
drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to function ‘lookup_address’
     lookup_address(addr, &level)).maddr;
     ^~~~~~~~~~~~~~
In file included from ./arch/arm64/include/asm/page.h:37:0, from ./include/linux/mmzone.h:20,
                  from ./include/linux/gfp.h:5,
                  from ./include/linux/mm.h:9,
                  from drivers/xen/xenbus/xenbus_client.c:33:
./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here extern pte_t *lookup_address(unsigned long addr); ^~~~~~~~~~~~~~

Quoted 3 lines

I've cc-d the xen folks, maybe they can suggest a way to untangle it? Alternatively, if someone can suggest a good naming scheme for the page levels, I can just do that.

The implementation of lookup_address(...) on ARM for Xen (see include/xen/arm/page.h) is just a BUG(). This is because this code should never be called (only used for x86 PV code).

Furthermore, xenbus client does not use at all the level. It is just to cope with the x86 version of lookup_address.

So one way to solve the problem would be to introduce xen_lookup_address(addr) that would be implemented as:
    - on x86
        unsigned int level;

        return lookup_address(addr, &level).maddr;
    - on ARM
        BUG();

With that there would be no prototype clash and avoid introducing a level parameter.

Hi,

CC Juergen, Boris and Stefano.

Quoted 38 lines · Tycho Andersen

Quoted 9 lines · Christoph Hellwig

Quoted 6 lines

+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */
+static pte_t *lookup_address(unsigned long addr)

Seems like this should be moved to common arm64 mm code and used by kernel_page_present.

Sounds good, I'll include something like the patch below in the next series.

Unfortunately, adding an implementation of lookup_address seems to be slightly more complicated than necessary, because of the xen piece. We have to define lookup_address() with the level parameter, but it's not obvious to me to name the page levels. So for now I've just left it as a WARN() if someone supplies it.

It seems like xen still does need this to be defined, because if I define it without level:

drivers/xen/xenbus/xenbus_client.c: In function ‘xenbus_unmap_ring_vfree_pv’:
drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to function ‘lookup_address’
     lookup_address(addr, &level)).maddr;
     ^~~~~~~~~~~~~~
In file included from ./arch/arm64/include/asm/page.h:37:0, from ./include/linux/mmzone.h:20,
                  from ./include/linux/gfp.h:5,
                  from ./include/linux/mm.h:9,
                  from drivers/xen/xenbus/xenbus_client.c:33:
./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here extern pte_t *lookup_address(unsigned long addr); ^~~~~~~~~~~~~~

Quoted 3 lines

I've cc-d the xen folks, maybe they can suggest a way to untangle it? Alternatively, if someone can suggest a good naming scheme for the page levels, I can just do that.

The implementation of lookup_address(...) on ARM for Xen (see include/xen/arm/page.h) is just a BUG(). This is because this code should never be called (only used for x86 PV code).

Furthermore, xenbus client does not use at all the level. It is just to cope with the x86 version of lookup_address.

So one way to solve the problem would be to introduce xen_lookup_address(addr) that would be implemented as:
    - on x86
        unsigned int level;

        return lookup_address(addr, &level).maddr;
    - on ARM
        BUG();

With that there would be no prototype clash and avoid introducing a level parameter.

Quoted 8 lines · Christoph Hellwig

Review

Quoted 5 lines

(quoted diff) +5 -0

+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */
+static pte_t *lookup_address(unsigned long addr)

Seems like this should be moved to common arm64 mm code and used by kernel_page_present.

Sounds good, I'll include something like the patch below in the next series.

Unfortunately, adding an implementation of lookup_address seems to be slightly more complicated than necessary, because of the xen piece. We have to define lookup_address() with the level parameter, but it's not obvious to me to name the page levels. So for now I've just left it as a WARN() if someone supplies it.

It seems like xen still does need this to be defined, because if I define it without level:

drivers/xen/xenbus/xenbus_client.c: In function ‘xenbus_unmap_ring_vfree_pv’:
drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to function ‘lookup_address’

     lookup_address(addr, &level)).maddr;
     ^~~~~~~~~~~~~~
In file included from ./arch/arm64/include/asm/page.h:37:0,
                  from ./include/linux/mmzone.h:20,
                  from ./include/linux/gfp.h:5,
                  from ./include/linux/mm.h:9,
                  from drivers/xen/xenbus/xenbus_client.c:33:
./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here
  extern pte_t *lookup_address(unsigned long addr);
                ^~~~~~~~~~~~~~

I've cc-d the xen folks, maybe they can suggest a way to untangle it? Alternatively, if someone can suggest a good naming scheme for the page levels, I can just do that.

Review

Quoted 5 lines

(quoted diff) +5 -0

+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */
+static pte_t *lookup_address(unsigned long addr)

Seems like this should be moved to common arm64 mm code and used by kernel_page_present.

From: Juerg Haefliger <[email protected]>

Enable support for eXclusive Page Frame Ownership (XPFO) for arm64 and provide a hook for updating a single kernel page table entry (which is required by the generic XPFO code).

v6: use flush_tlb_kernel_range() instead of __flush_tlb_one()

CC [email protected]

Signed-off-by Juerg Haefliger <[email protected]>

Signed-off-by Tycho Andersen <[email protected]>

---
 arch/arm64/Kconfig     |  1 +
 arch/arm64/mm/Makefile |  2 ++
 arch/arm64/mm/xpfo.c   | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

arch/arm64/Kconfig +0 -0

index dfd908630631..44fa44ef02ec 100644

arch/arm64/Kconfig +0 -0

 	help
 	  ARM 64-bit (AArch64) Linux support.
 

arch/arm64/mm/Makefile +2 -0

index 9b0ba191e48e..22e5cab543d8 100644
@@ -11,3 +11,5 @@ KASAN_SANITIZE_physaddr.o	+= n
 
 obj-$(CONFIG_KASAN)		+= kasan_init.o
 KASAN_SANITIZE_kasan_init.o	:= n
+
+obj-$(CONFIG_XPFO)		+= xpfo.o

arch/arm64/mm/xpfo.c new file +12 -0

new file mode 100644
index 000000000000..678e2be848eb
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2017 Hewlett Packard Enterprise Development, L.P.
+ * Copyright (C) 2016 Brown University. All rights reserved.
+ *
+ * Authors:
+ *   Juerg Haefliger <[email protected]>
+ *   Vasileios P. Kemerlis <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */

Hi,

Quoted 22 lines · Tycho Andersen

From: Juerg Haefliger <[email protected]>

Enable support for eXclusive Page Frame Ownership (XPFO) for arm64 and provide a hook for updating a single kernel page table entry (which is required by the generic XPFO code).

v6: use flush_tlb_kernel_range() instead of __flush_tlb_one()

CC [email protected]

Signed-off-by Juerg Haefliger <[email protected]>

Signed-off-by Tycho Andersen <[email protected]>

---
 arch/arm64/Kconfig     |  1 +
 arch/arm64/mm/Makefile |  2 ++
 arch/arm64/mm/xpfo.c   | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

arch/arm64/Kconfig +0 -0

index dfd908630631..44fa44ef02ec 100644

arch/arm64/Kconfig config ARM64 · line 124

Quoted 4 lines

arch/arm64/Kconfig +1 -0

@@ -121,6 +121,7 @@ config ARM64
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
 	select THREAD_INFO_IN_TASK
+	select ARCH_SUPPORTS_XPFO

A bit of a nit, but this list is (intended to be) organised alphabetically. Could you please try to retain that?

i.e. place this between ARCH_SUPPORTS_NUMA_BALANCING and ARCH_WANT_COMPAT_IPC_PARSE_VERSION.

Quoted 31 lines

arch/arm64/Kconfig +0 -0

 	help
 	  ARM 64-bit (AArch64) Linux support.
 

arch/arm64/mm/Makefile +2 -0

index 9b0ba191e48e..22e5cab543d8 100644
@@ -11,3 +11,5 @@ KASAN_SANITIZE_physaddr.o	+= n
 
 obj-$(CONFIG_KASAN)		+= kasan_init.o
 KASAN_SANITIZE_kasan_init.o	:= n
+
+obj-$(CONFIG_XPFO)		+= xpfo.o

arch/arm64/mm/xpfo.c new file +12 -0

new file mode 100644
index 000000000000..678e2be848eb
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2017 Hewlett Packard Enterprise Development, L.P.
+ * Copyright (C) 2016 Brown University. All rights reserved.
+ *
+ * Authors:
+ *   Juerg Haefliger <[email protected]>
+ *   Vasileios P. Kemerlis <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */

arch/arm64/mm/xpfo.c line 22

Quoted 10 lines

arch/arm64/mm/xpfo.c new file +10 -0

+
+#include <linux/mm.h>
+#include <linux/module.h>
+
+#include <asm/tlbflush.h>
+
+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */

Is this intended for kernel VAs, user VAs, or both?

There are different constraints for fiddling with either (e.g. holding mmap_sem), so we should be clear regarding the intended use-case.

arch/arm64/mm/xpfo.c line 35

Quoted 13 lines

arch/arm64/mm/xpfo.c new file +13 -0

+static pte_t *lookup_address(unsigned long addr)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+
+	pgd = pgd_offset_k(addr);
+	if (pgd_none(*pgd))
+		return NULL;
+
+	pud = pud_offset(pgd, addr);
+	if (pud_none(*pud))
+		return NULL;

What if it's not none, but not a table?

I think we chould check pud_sect() here, and/or pud_bad().

arch/arm64/mm/xpfo.c line 39

Quoted 4 lines

arch/arm64/mm/xpfo.c new file +4 -0

+
+	pmd = pmd_offset(pud, addr);
+	if (pmd_none(*pmd))
+		return NULL;

Likewise.

arch/arm64/mm/xpfo.c line 42

Quoted 3 lines

arch/arm64/mm/xpfo.c new file +3 -0

+
+	return pte_offset_kernel(pmd, addr);
+}

Given this expects a pte, it might make more sense to call this lookup_address_pte() to make that clear.

arch/arm64/mm/xpfo.c line 49

Quoted 7 lines

arch/arm64/mm/xpfo.c new file +7 -0

+
+/* Update a single kernel page table entry */
+inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
+{
+	pte_t *pte = lookup_address((unsigned long)kaddr);
+
+	set_pte(pte, pfn_pte(page_to_pfn(page), prot));

We can get NULL from lookup_address(), so this doesn't look right.

If NULL implies an error, drop a BUG_ON(!pte) before the set_pte.

arch/arm64/mm/xpfo.c line 55

Quoted 6 lines

arch/arm64/mm/xpfo.c new file +6 -0

+}
+
+inline void xpfo_flush_kernel_tlb(struct page *page, int order)
+{
+	unsigned long kaddr = (unsigned long)page_address(page);
+	unsigned long size = PAGE_SIZE;

unsigned long size = PAGE_SIZE << order;

arch/arm64/mm/xpfo.c line 57

Quoted 2 lines

arch/arm64/mm/xpfo.c new file +2 -0

+
+	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);

... and this can be simpler.

I haven't brought myself back up to speed, so it might not be possible, but I still think it would be preferable for XPFO to call flush_tlb_kernel_range() directly.

From: Juerg Haefliger <[email protected]>

If the page is unmapped by XPFO, a data cache flush results in a fatal page fault, so let's temporarily map the region, flush the cache, and then unmap it.

v6: actually flush in the face of xpfo, and temporarily map the underlying
memory so it can be flushed correctly

CC [email protected]

Signed-off-by Juerg Haefliger <[email protected]>

Signed-off-by Tycho Andersen <[email protected]>

---
 arch/arm64/mm/flush.c | 7 +++++++
 1 file changed, 7 insertions(+)

arch/arm64/mm/flush.c +1 -0

index 21a8d828cbf4..e335e3fd4fca 100644
@@ -20,6 +20,7 @@
 #include <linux/export.h>
 #include <linux/mm.h>
 #include <linux/pagemap.h>
+#include <linux/xpfo.h>
 
 #include <asm/cacheflush.h>
 #include <asm/cache.h>

@@ -28,9 +29,15 @@
 void sync_icache_aliases(void *kaddr, unsigned long len)
 {

Quoted 31 lines · Tycho Andersen

From: Juerg Haefliger <[email protected]>

If the page is unmapped by XPFO, a data cache flush results in a fatal page fault, so let's temporarily map the region, flush the cache, and then unmap it.

v6: actually flush in the face of xpfo, and temporarily map the underlying
memory so it can be flushed correctly

CC [email protected]

Signed-off-by Juerg Haefliger <[email protected]>

Signed-off-by Tycho Andersen <[email protected]>

---
 arch/arm64/mm/flush.c | 7 +++++++
 1 file changed, 7 insertions(+)

arch/arm64/mm/flush.c +1 -0

index 21a8d828cbf4..e335e3fd4fca 100644
@@ -20,6 +20,7 @@
 #include <linux/export.h>
 #include <linux/mm.h>
 #include <linux/pagemap.h>
+#include <linux/xpfo.h>
 
 #include <asm/cacheflush.h>
 #include <asm/cache.h>

@@ -28,9 +29,15 @@
 void sync_icache_aliases(void *kaddr, unsigned long len)
 {

arch/arm64/mm/flush.c line 40

Quoted 10 lines

arch/arm64/mm/flush.c +6 -0

 	unsigned long addr = (unsigned long)kaddr;
+	unsigned long num_pages = XPFO_NUM_PAGES(addr, len);
+	void *mapping[num_pages];
 
 	if (icache_is_aliasing()) {
+		xpfo_temp_map(kaddr, len, mapping,
+			      sizeof(mapping[0]) * num_pages);
 		__clean_dcache_area_pou(kaddr, len);
+		xpfo_temp_unmap(kaddr, len, mapping,
+			        sizeof(mapping[0]) * num_pages);

Does this create the mapping in-place?