Add support for eXclusive Page Frame Ownership

From: Tycho Andersen <[email protected]>

Oopsing might kill the task, via rewind_stack_do_exit() at the bottom, and that might sleep:

arch/x86/mm/fault.c +0 -0

index 9d5c75f02295..7891add0913f 100644
@@ -858,6 +858,12 @@ no_context(struct pt_regs *regs, unsigned long error_code,
        /* Executive summary in case the body of the oops scrolled away */
        printk(KERN_DEFAULT "CR2: %016lx\n", address);

Quoted 16 lines · Khalid Aziz

From: Tycho Andersen <[email protected]>

Oopsing might kill the task, via rewind_stack_do_exit() at the bottom, and that might sleep:

arch/x86/mm/fault.c +0 -0

index 9d5c75f02295..7891add0913f 100644
@@ -858,6 +858,12 @@ no_context(struct pt_regs *regs, unsigned long error_code,
        /* Executive summary in case the body of the oops scrolled away */
        printk(KERN_DEFAULT "CR2: %016lx\n", address);
 

arch/x86/mm/fault.c no_context(struct pt_regs *regs, unsigned long error_code, · line 869

Quoted 9 lines

arch/x86/mm/fault.c +6 -0

+       /*
+        * We're about to oops, which might kill the task. Make sure we're
+        * allowed to sleep.
+        */
+       flags |= X86_EFLAGS_IF;
+
        oops_end(flags, regs, sig);
 }
 

NAK. If there's a bug in rewind_stack_do_exit(), please fix it in rewind_stack_do_exit().

Quoted 17 lines · Andy Lutomirski

Quoted 16 lines · Khalid Aziz

From: Tycho Andersen <[email protected]>

Oopsing might kill the task, via rewind_stack_do_exit() at the bottom, and that might sleep:

arch/x86/mm/fault.c +0 -0

index 9d5c75f02295..7891add0913f 100644
@@ -858,6 +858,12 @@ no_context(struct pt_regs *regs, unsigned long error_code,
        /* Executive summary in case the body of the oops scrolled away */
        printk(KERN_DEFAULT "CR2: %016lx\n", address);
 

Quoted 13 lines

arch/x86/mm/fault.c no_context(struct pt_regs *regs, unsigned long error_code, · line 869

Quoted 9 lines

arch/x86/mm/fault.c +6 -0

+       /*
+        * We're about to oops, which might kill the task. Make sure we're
+        * allowed to sleep.
+        */
+       flags |= X86_EFLAGS_IF;
+
        oops_end(flags, regs, sig);
 }
 

NAK. If there's a bug in rewind_stack_do_exit(), please fix it in rewind_stack_do_exit().

[I trimmed the CC list since google rejected it with E2BIG :)]

I guess the problem is really that do_exit() (or really exit_signals()) might sleep. Maybe we should put an irq_enable() at the beginning of do_exit() instead and fix this problem for all arches?

Quoted 33 lines · Tycho Andersen

Quoted 17 lines · Andy Lutomirski

Quoted 16 lines · Khalid Aziz

From: Tycho Andersen <[email protected]>

Oopsing might kill the task, via rewind_stack_do_exit() at the bottom, and that might sleep:

arch/x86/mm/fault.c +0 -0

index 9d5c75f02295..7891add0913f 100644
@@ -858,6 +858,12 @@ no_context(struct pt_regs *regs, unsigned long error_code,
        /* Executive summary in case the body of the oops scrolled away */
        printk(KERN_DEFAULT "CR2: %016lx\n", address);
 

Quoted 13 lines

arch/x86/mm/fault.c no_context(struct pt_regs *regs, unsigned long error_code, · line 869

Quoted 9 lines

arch/x86/mm/fault.c +6 -0

+       /*
+        * We're about to oops, which might kill the task. Make sure we're
+        * allowed to sleep.
+        */
+       flags |= X86_EFLAGS_IF;
+
        oops_end(flags, regs, sig);
 }
 

NAK. If there's a bug in rewind_stack_do_exit(), please fix it in rewind_stack_do_exit().

Quoted 7 lines

[I trimmed the CC list since google rejected it with E2BIG :)]

I guess the problem is really that do_exit() (or really exit_signals()) might sleep. Maybe we should put an irq_enable() at the beginning of do_exit() instead and fix this problem for all arches?

Hmm. do_exit() isn't really meant to be "try your best to leave the system somewhat usable without returning" -- it's a function that, other than in OOPSes, is called from a well-defined state. So I think rewind_stack_do_exit() is probably a better spot. But we need to rewind the stack and *then* turn on IRQs, since we otherwise risk exploding quite badly.

From 34dce229a4f43f90db823671eb0b8da7c4906045 Mon Sep 17 00:00:00 2001

Quoted 6 lines

Hmm. do_exit() isn't really meant to be "try your best to leave the system somewhat usable without returning" -- it's a function that, other than in OOPSes, is called from a well-defined state. So I think rewind_stack_do_exit() is probably a better spot. But we need to rewind the stack and *then* turn on IRQs, since we otherwise risk exploding quite badly.

Ok, sounds good. I guess we can include something like this patch in the next series.

Thanks,

Tycho

arch/x86/entry/entry_32.S ENTRY(rewind_stack_do_exit) · line 1514

Quoted 10 lines · Tycho Andersen

leaq -PTREGS_SIZE(%rax), %rsp
UNWIND_HINT_FUNC sp_offset=PTREGS_SIZE

arch/x86/entry/entry_32.S +5 -0

+    /*
+     * If we oopsed in an interrupt handler, interrupts may be off. Let's turn
+     * them back on before going back to "normal" code.
+     */
+    sti

That breaks the paravirt muck and tracing/lockdep.

ENABLE_INTERRUPTS() is what you want plus TRACE_IRQ_ON to keep the tracer and lockdep happy.

include/linux/mm_types.h +0 -0

index 2c471a2c43fa..d17d33f36a01 100644

include/linux/mm_types.h +0 -0

 } _struct_page_alignment;

Quoted 5 lines · Khalid Aziz

include/linux/mm_types.h +0 -0

index 2c471a2c43fa..d17d33f36a01 100644

include/linux/mm_types.h struct page { · line 213

Quoted 10 lines

include/linux/mm_types.h +7 -0

@@ -204,6 +204,14 @@ struct page {
 #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
 	int _last_cpupid;
 #endif
+
+#ifdef CONFIG_XPFO
+	/* Counts the number of times this page has been kmapped. */
+	atomic_t xpfo_mapcount;
+
+	/* Serialize kmap/kunmap of this page */
+	spinlock_t xpfo_lock;

NAK, see ALLOC_SPLIT_PTLOCKS

spinlock_t can be _huge_ (CONFIG_PROVE_LOCKING=y), also are you _really_ sure you want spinlock_t and not raw_spinlock_t ? For CONFIG_PREEMPT_FULL spinlock_t turns into a rtmutex.

include/linux/mm_types.h struct page { · line 214

Quoted 1 line

include/linux/mm_types.h +1 -0

+#endif

Growing the page-frame by 8 bytes (in the good case) is really sad, that's a _lot_ of memory.

Quoted 5 lines · Khalid Aziz

include/linux/mm_types.h +0 -0

index 2c471a2c43fa..d17d33f36a01 100644

include/linux/mm_types.h struct page { · line 213

Quoted 10 lines

include/linux/mm_types.h +7 -0

@@ -204,6 +204,14 @@ struct page {
 #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
 	int _last_cpupid;
 #endif
+
+#ifdef CONFIG_XPFO
+	/* Counts the number of times this page has been kmapped. */
+	atomic_t xpfo_mapcount;
+
+	/* Serialize kmap/kunmap of this page */
+	spinlock_t xpfo_lock;

NAK, see ALLOC_SPLIT_PTLOCKS

spinlock_t can be _huge_ (CONFIG_PROVE_LOCKING=y), also are you _really_ sure you want spinlock_t and not raw_spinlock_t ? For CONFIG_PREEMPT_FULL spinlock_t turns into a rtmutex.

include/linux/mm_types.h struct page { · line 214

Quoted 1 line

include/linux/mm_types.h +1 -0

+#endif

Growing the page-frame by 8 bytes (in the good case) is really sad, that's a _lot_ of memory.

include/linux/xpfo.h new file +11 -0

+static inline void xpfo_kmap(void *kaddr, struct page *page)
+{
+	unsigned long flags;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	if (!PageXpfoUser(page))
+		return;
+
+	/*

include/linux/xpfo.h new file +15 -0

+}
+
+static inline void xpfo_kunmap(void *kaddr, struct page *page)
+{
+	unsigned long flags;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	if (!PageXpfoUser(page))
+		return;
+
+	/*
+	 * The page is to be allocated back to user space, so unmap it from
+	 * the kernel, flush the TLB and tag it as a user page.

+}

You must be so glad I no longer use kmap_atomic from NMI context :-)

Quoted 11 lines · Khalid Aziz

include/linux/xpfo.h new file +11 -0

+static inline void xpfo_kmap(void *kaddr, struct page *page)
+{
+	unsigned long flags;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	if (!PageXpfoUser(page))
+		return;
+
+	/*

include/linux/xpfo.h line 56

Quoted 10 lines

include/linux/xpfo.h new file +10 -0

+	 * The page was previously allocated to user space, so
+	 * map it back into the kernel if needed. No TLB flush required.
+	 */
+	spin_lock_irqsave(&page->xpfo_lock, flags);
+
+	if ((atomic_inc_return(&page->xpfo_mapcount) == 1) &&
+		TestClearPageXpfoUnmapped(page))
+		set_kpte(kaddr, page, PAGE_KERNEL);
+
+	spin_unlock_irqrestore(&page->xpfo_lock, flags);

That's a really sad sequence, not wrong, but sad. _3_ atomic operations, 2 on likely the same cacheline. And mostly all pointless.

This patch makes xpfo_mapcount an atomic, but then all modifications are under the spinlock, what gives?

Anyway, a possibly saner sequence might be:

	if (atomic_inc_not_zero(&page->xpfo_mapcount))
		return;

	spin_lock_irqsave(&page->xpfo_lock, flag);
	if ((atomic_inc_return(&page->xpfo_mapcount) == 1) &&
	    TestClearPageXpfoUnmapped(page))
		set_kpte(kaddr, page, PAGE_KERNEL);
	spin_unlock_irqrestore(&page->xpfo_lock, flags);

Quoted 15 lines

include/linux/xpfo.h new file +15 -0

+}
+
+static inline void xpfo_kunmap(void *kaddr, struct page *page)
+{
+	unsigned long flags;
+
+	if (!static_branch_unlikely(&xpfo_inited))
+		return;
+
+	if (!PageXpfoUser(page))
+		return;
+
+	/*
+	 * The page is to be allocated back to user space, so unmap it from
+	 * the kernel, flush the TLB and tag it as a user page.

include/linux/xpfo.h line 81

Quoted 10 lines

include/linux/xpfo.h new file +10 -0

+	 */
+	spin_lock_irqsave(&page->xpfo_lock, flags);
+
+	if (atomic_dec_return(&page->xpfo_mapcount) == 0) {
+#ifdef CONFIG_XPFO_DEBUG
+		WARN_ON(PageXpfoUnmapped(page));
+#endif
+		SetPageXpfoUnmapped(page);
+		set_kpte(kaddr, page, __pgprot(0));
+		xpfo_flush_kernel_tlb(page, 0);

You didn't speak about the TLB invalidation anywhere. But basically this is one that x86 cannot do.

include/linux/xpfo.h line 84

Quoted 3 lines

include/linux/xpfo.h new file +3 -0

+	}
+
+	spin_unlock_irqrestore(&page->xpfo_lock, flags);

Idem:

	if (atomic_add_unless(&page->xpfo_mapcount, -1, 1))
		return;

	....

Quoted 1 line

+}

Also I'm failing to see the point of PG_xpfo_unmapped, afaict it is identical to !atomic_read(&page->xpfo_mapcount).

Thanks Peter. I really appreciate your review. Your feedback helps make this code better and closer to where I can feel comfortable not calling it RFC any more.

The more I look at xpfo_kmap()/xpfo_kunmap() code, the more I get uncomfortable with it. As you pointed out about calling kmap_atomic from NMI context, that just makes the kmap_atomic code look even worse. I pointed out one problem with this code in cover letter and suggested a rewrite. I see these problems with this code:

1. When xpfo_kmap maps a page back in physmap, it opens up the ret2dir attack security hole again even if just for the duration of kmap. A kmap can stay around for some time if the page is being used for I/O.

2. This code uses spinlock which leads to problems. If it does not disable IRQ, it is exposed to deadlock around xpfo_lock. If it disables IRQ, I think it can still deadlock around pgd_lock.

I think a better implementation of xpfo_kmap()/xpfo_kunmap() would map the page at a new virtual address similar to what kmap_high for i386 does. This avoids re-opening the ret2dir security hole. We can also possibly do away with xpfo_lock saving bytes in page-frame and the not so sane code sequence can go away.

From: Juerg Haefliger <[email protected]>

This patch adds basic support infrastructure 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. Individual architectures can enable full XPFO support using this infrastructure by supplying architecture specific pieces.

[ Sorry, had to trim the Cc: list from hell. Tried to keep all the
mailing lists and all x86 developers. ]

Quoted 10 lines · Khalid Aziz

From: Juerg Haefliger <[email protected]>

This patch adds basic support infrastructure 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. Individual architectures can enable full XPFO support using this infrastructure by supplying architecture specific pieces.

I have a higher level, meta question:

Is there any updated analysis outlining why this XPFO overhead would be required on x86-64 kernels running on SMAP/SMEP CPUs which should be all recent Intel and AMD CPUs, and with kernel that mark all direct kernel mappings as non-executable - which should be all reasonably modern kernels later than v4.0 or so?

I.e. the original motivation of the XPFO patches was to prevent execution of direct kernel mappings. Is this motivation still present if those mappings are non-executable?

(Sorry if this has been asked and answered in previous discussions.)

Quoted 5 lines

I.e. the original motivation of the XPFO patches was to prevent execution of direct kernel mappings. Is this motivation still present if those mappings are non-executable?

(Sorry if this has been asked and answered in previous discussions.)

Hi Ingo,

That is a good question. Because of the cost of XPFO, we have to be very sure we need this protection. The paper from Vasileios, Michalis and Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1 and 6.2.

Quoted 6 lines · Khalid Aziz

Quoted 5 lines

I.e. the original motivation of the XPFO patches was to prevent execution of direct kernel mappings. Is this motivation still present if those mappings are non-executable?

(Sorry if this has been asked and answered in previous discussions.)

Quoted 7 lines

Hi Ingo,

That is a good question. Because of the cost of XPFO, we have to be very sure we need this protection. The paper from Vasileios, Michalis and Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1 and 6.2.

So it would be nice if you could generally summarize external arguments when defending a patchset, instead of me having to dig through a PDF which not only causes me to spend time that you probably already spent reading that PDF, but I might also interpret it incorrectly. ;-)

The PDF you cited says this:

"Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
in many platforms, including x86-64. In our example, the content of user address 0xBEEF000 is also accessible through kernel address 0xFFFF87FF9F080000 as plain, executable code."

Is this actually true of modern x86-64 kernels? We've locked down W^X protections in general.

Quoted 26 lines · Ingo Molnar

Quoted 6 lines · Khalid Aziz

Quoted 5 lines

I.e. the original motivation of the XPFO patches was to prevent execution of direct kernel mappings. Is this motivation still present if those mappings are non-executable?

(Sorry if this has been asked and answered in previous discussions.)

Quoted 7 lines

Hi Ingo,

That is a good question. Because of the cost of XPFO, we have to be very sure we need this protection. The paper from Vasileios, Michalis and Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1 and 6.2.

So it would be nice if you could generally summarize external arguments when defending a patchset, instead of me having to dig through a PDF which not only causes me to spend time that you probably already spent reading that PDF, but I might also interpret it incorrectly. ;-)

The PDF you cited says this:

Quoted 7 lines

"Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
in many platforms, including x86-64. In our example, the content of user address 0xBEEF000 is also accessible through kernel address 0xFFFF87FF9F080000 as plain, executable code."

Is this actually true of modern x86-64 kernels? We've locked down W^X protections in general.

As I was curious, I looked at the paper. Here is a quote from it:

"In x86-64, however, the permissions of physmap are not in sane state. Kernels up to v3.8.13 violate the W^X property by mapping the entire region as “readable, writeable, and executable” (RWX)—only very recent kernels (≥v3.9) use the more conservative RW mapping.”

Is this actually true of modern x86-64 kernels? We've locked down W^X protections in general.

I.e. this conclusion:

"Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
triggering the kernel to dereference it, an attacker can directly execute shell code with kernel privileges."

... appears to be predicated on imperfect W^X protections on the x86-64 kernel.

Do such holes exist on the latest x86-64 kernel? If yes, is there a reason to believe that these W^X holes cannot be fixed, or that any fix would be more expensive than XPFO?

Quoted 34 lines · Nadav Amit

Quoted 26 lines · Ingo Molnar

Quoted 6 lines · Khalid Aziz

Quoted 5 lines

I.e. the original motivation of the XPFO patches was to prevent execution of direct kernel mappings. Is this motivation still present if those mappings are non-executable?

(Sorry if this has been asked and answered in previous discussions.)

Quoted 7 lines

Hi Ingo,

That is a good question. Because of the cost of XPFO, we have to be very sure we need this protection. The paper from Vasileios, Michalis and Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1 and 6.2.

So it would be nice if you could generally summarize external arguments when defending a patchset, instead of me having to dig through a PDF which not only causes me to spend time that you probably already spent reading that PDF, but I might also interpret it incorrectly. ;-)

The PDF you cited says this:

Quoted 7 lines

"Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
in many platforms, including x86-64. In our example, the content of user address 0xBEEF000 is also accessible through kernel address 0xFFFF87FF9F080000 as plain, executable code."

Is this actually true of modern x86-64 kernels? We've locked down W^X protections in general.

Quoted 6 lines

As I was curious, I looked at the paper. Here is a quote from it:

"In x86-64, however, the permissions of physmap are not in sane state. Kernels up to v3.8.13 violate the W^X property by mapping the entire region as “readable, writeable, and executable” (RWX)—only very recent kernels (≥v3.9) use the more conservative RW mapping.”

But v3.8.13 is a 5+ years old kernel, it doesn't count as a "modern" kernel in any sense of the word. For any proposed patchset with significant complexity and non-trivial costs the benchmark version threshold is the "current upstream kernel".

So does that quote address my followup questions:

Quoted 9 lines

Is this actually true of modern x86-64 kernels? We've locked down W^X protections in general.

I.e. this conclusion:

"Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
triggering the kernel to dereference it, an attacker can directly execute shell code with kernel privileges."

Quoted 6 lines

... appears to be predicated on imperfect W^X protections on the x86-64 kernel.

Do such holes exist on the latest x86-64 kernel? If yes, is there a reason to believe that these W^X holes cannot be fixed, or that any fix would be more expensive than XPFO?

?

What you are proposing here is a XPFO patch-set against recent kernels with significant runtime overhead, so my questions about the W^X holes are warranted.

Quoted 33 lines · Ingo Molnar

Quoted 6 lines

As I was curious, I looked at the paper. Here is a quote from it:

"In x86-64, however, the permissions of physmap are not in sane state. Kernels up to v3.8.13 violate the W^X property by mapping the entire region as “readable, writeable, and executable” (RWX)—only very recent kernels (≥v3.9) use the more conservative RW mapping.”

But v3.8.13 is a 5+ years old kernel, it doesn't count as a "modern" kernel in any sense of the word. For any proposed patchset with significant complexity and non-trivial costs the benchmark version threshold is the "current upstream kernel".

So does that quote address my followup questions:

Quoted 9 lines

Is this actually true of modern x86-64 kernels? We've locked down W^X protections in general.

I.e. this conclusion:

"Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
triggering the kernel to dereference it, an attacker can directly execute shell code with kernel privileges."

Quoted 6 lines

... appears to be predicated on imperfect W^X protections on the x86-64 kernel.

Do such holes exist on the latest x86-64 kernel? If yes, is there a reason to believe that these W^X holes cannot be fixed, or that any fix would be more expensive than XPFO?

?

Quoted 4 lines

What you are proposing here is a XPFO patch-set against recent kernels with significant runtime overhead, so my questions about the W^X holes are warranted.

Just to clarify - I am an innocent bystander and have no part in this work. I was just looking (again) at the paper, as I was curious due to the recent patches that I sent that improve W^X protection.

It's not necessarily a W+X issue. The user space text is mapped in the kernel as well and even if it is mapped RX then this can happen. So any kernel mappings of user space text need to be mapped NX!

Quoted 4 lines · On Wed, Apr 17, 2019, 14:20 Thomas Gleixner <[email protected]> wrote:

It's not necessarily a W+X issue. The user space text is mapped in the kernel as well and even if it is mapped RX then this can happen. So any kernel mappings of user space text need to be mapped NX!

With SMEP, user space pages are always NX.

From the top of my head I'd say this is a non issue as those kernel address

Quoted 1 line · Linus Torvalds

With SMEP, user space pages are always NX.

We talk past each other. The user space page in the ring3 valid virtual address space (non negative) is of course protected by SMEP.

The attack utilizes the kernel linear mapping of the physical memory. I.e. user space address 0x43210 has a kernel equivalent at 0xfxxxxxxxxxx. So if the attack manages to trick the kernel to that valid kernel address and that is mapped X --> game over. SMEP does not help there.

From the top of my head I'd say this is a non issue as those kernel address space mappings _should_ be NX, but we got bitten by _should_ in the past:)

Quoted 5 lines · Thomas Gleixner

Quoted 1 line · Linus Torvalds

With SMEP, user space pages are always NX.

Quoted 8 lines

We talk past each other. The user space page in the ring3 valid virtual address space (non negative) is of course protected by SMEP.

The attack utilizes the kernel linear mapping of the physical memory. I.e. user space address 0x43210 has a kernel equivalent at 0xfxxxxxxxxxx. So if the attack manages to trick the kernel to that valid kernel address and that is mapped X --> game over. SMEP does not help there.

Oh, agreed.

But that would simply be a kernel bug. We should only map kernel pages executable when we have kernel code in them, and we should certainly not allow those pages to be mapped writably in user space.

That kind of "executable in kernel, writable in user" would be a horrendous and major bug.

So i think it's a non-issue.

Quoted 2 lines

From the top of my head I'd say this is a non issue as those kernel address space mappings _should_ be NX, but we got bitten by _should_ in the past:)

I do agree that bugs can happen, obviously, and we might have missed something.

But in the context of XPFO, I would argue (*very* strongly) that the likelihood of the above kind of bug is absolutely *miniscule* compared to the likelihood that we'd have something wrong in the software implementation of XPFO.

So if the argument is "we might have bugs in software", then I think that's an argument _against_ XPFO rather than for it.

I don't think this type of NX goof was ever the argument for XPFO. The main argument I've heard is that a malicious user program writes a ROP payload into user memory (regular anonymous user memory) and then gets the kernel to erroneously set RSP (*not* RIP) to point there.

I find this argument fairly weak for a couple reasons. First, if we're worried about this, let's do in-kernel CFI, not XPFO, to

mitigate it. Second, I don't see why the exact same attack can't be done using, say, page cache, and unless I'm missing something, XPFO doesn't protect page cache. Or network buffers, or pipe buffers, etc.

Quoted 4 lines · Andy Lutomirski

I don't think this type of NX goof was ever the argument for XPFO. The main argument I've heard is that a malicious user program writes a ROP payload into user memory (regular anonymous user memory) and then gets the kernel to erroneously set RSP (*not* RIP) to point there.

Well, more than just ROP. Any of the various attack primitives. The NX stuff is about moving RIP: SMEP-bypassing. But there is still basic SMAP-bypassing for putting a malicious structure in userspace and having the kernel access it via the linear mapping, etc.

Quoted 2 lines

I find this argument fairly weak for a couple reasons. First, if we're worried about this, let's do in-kernel CFI, not XPFO, to

CFI is getting much closer. Getting the kernel happy under Clang, LTO, and CFI is under active development. (It's functional for arm64 already, and pieces have been getting upstreamed.)

Quoted 11 lines · Kees Cook

Quoted 4 lines · Andy Lutomirski

I don't think this type of NX goof was ever the argument for XPFO. The main argument I've heard is that a malicious user program writes a ROP payload into user memory (regular anonymous user memory) and then gets the kernel to erroneously set RSP (*not* RIP) to point there.

Well, more than just ROP. Any of the various attack primitives. The NX stuff is about moving RIP: SMEP-bypassing. But there is still basic SMAP-bypassing for putting a malicious structure in userspace and having the kernel access it via the linear mapping, etc.

Quoted 7 lines

Quoted 2 lines

I find this argument fairly weak for a couple reasons. First, if we're worried about this, let's do in-kernel CFI, not XPFO, to

CFI is getting much closer. Getting the kernel happy under Clang, LTO, and CFI is under active development. (It's functional for arm64 already, and pieces have been getting upstreamed.)

CFI theoretically offers protection with fairly low overhead. I have not played much with CFI in clang. I agree with Linus that probability of bugs in XPFO implementation itself is a cause of concern. If CFI in Clang can provide us the same level of protection as XPFO does, I wouldn't want to push for an expensive change like XPFO.

If Clang/CFI can't get us there for extended period of time, does it make sense to continue to poke at XPFO?

Quoted 11 lines · Kees Cook

Quoted 4 lines · Andy Lutomirski

I don't think this type of NX goof was ever the argument for XPFO. The main argument I've heard is that a malicious user program writes a ROP payload into user memory (regular anonymous user memory) and then gets the kernel to erroneously set RSP (*not* RIP) to point there.

Well, more than just ROP. Any of the various attack primitives. The NX stuff is about moving RIP: SMEP-bypassing. But there is still basic SMAP-bypassing for putting a malicious structure in userspace and having the kernel access it via the linear mapping, etc.

Quoted 7 lines

Quoted 2 lines

I find this argument fairly weak for a couple reasons. First, if we're worried about this, let's do in-kernel CFI, not XPFO, to

CFI is getting much closer. Getting the kernel happy under Clang, LTO, and CFI is under active development. (It's functional for arm64 already, and pieces have been getting upstreamed.)

CFI theoretically offers protection with fairly low overhead. I have not played much with CFI in clang. I agree with Linus that probability of bugs in XPFO implementation itself is a cause of concern. If CFI in Clang can provide us the same level of protection as XPFO does, I wouldn't want to push for an expensive change like XPFO.

If Clang/CFI can't get us there for extended period of time, does it make sense to continue to poke at XPFO?

Quoted 11 lines · Thomas Gleixner

Quoted 1 line · Linus Torvalds

With SMEP, user space pages are always NX.

We talk past each other. The user space page in the ring3 valid virtual address space (non negative) is of course protected by SMEP.

The attack utilizes the kernel linear mapping of the physical memory. I.e. user space address 0x43210 has a kernel equivalent at 0xfxxxxxxxxxx. So if the attack manages to trick the kernel to that valid kernel address and that is mapped X --> game over. SMEP does not help there.

Oh, agreed.

But that would simply be a kernel bug. We should only map kernel pages executable when we have kernel code in them, and we should certainly not allow those pages to be mapped writably in user space.

That kind of "executable in kernel, writable in user" would be a horrendous and major bug.

So i think it's a non-issue.

Quoted 2 lines

From the top of my head I'd say this is a non issue as those kernel address space mappings _should_ be NX, but we got bitten by _should_ in the past:)

I do agree that bugs can happen, obviously, and we might have missed something.

But in the context of XPFO, I would argue (*very* strongly) that the likelihood of the above kind of bug is absolutely *miniscule* compared to the likelihood that we'd have something wrong in the software implementation of XPFO.

So if the argument is "we might have bugs in software", then I think that's an argument _against_ XPFO rather than for it.

Quoted 6 lines · Khalid Aziz

Quoted 5 lines

I.e. the original motivation of the XPFO patches was to prevent execution of direct kernel mappings. Is this motivation still present if those mappings are non-executable?

(Sorry if this has been asked and answered in previous discussions.)

Quoted 7 lines

Hi Ingo,

That is a good question. Because of the cost of XPFO, we have to be very sure we need this protection. The paper from Vasileios, Michalis and Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1 and 6.2.

So it would be nice if you could generally summarize external arguments when defending a patchset, instead of me having to dig through a PDF which not only causes me to spend time that you probably already spent reading that PDF, but I might also interpret it incorrectly. ;-)

The PDF you cited says this:

"Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
in many platforms, including x86-64. In our example, the content of user address 0xBEEF000 is also accessible through kernel address 0xFFFF87FF9F080000 as plain, executable code."

Is this actually true of modern x86-64 kernels? We've locked down W^X protections in general.

I.e. this conclusion:

"Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
triggering the kernel to dereference it, an attacker can directly execute shell code with kernel privileges."

... appears to be predicated on imperfect W^X protections on the x86-64 kernel.

Do such holes exist on the latest x86-64 kernel? If yes, is there a reason to believe that these W^X holes cannot be fixed, or that any fix would be more expensive than XPFO?

Quoted 17 lines · Ingo Molnar

Quoted 6 lines · Khalid Aziz

Quoted 5 lines

I.e. the original motivation of the XPFO patches was to prevent execution of direct kernel mappings. Is this motivation still present if those mappings are non-executable?

(Sorry if this has been asked and answered in previous discussions.)

Quoted 7 lines

Hi Ingo,

That is a good question. Because of the cost of XPFO, we have to be very sure we need this protection. The paper from Vasileios, Michalis and Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1 and 6.2.

Quoted 4 lines

So it would be nice if you could generally summarize external arguments when defending a patchset, instead of me having to dig through a PDF which not only causes me to spend time that you probably already spent reading that PDF, but I might also interpret it incorrectly. ;-)

Sorry, you are right. Even though that paper explains it well, a summary is always useful.

Quoted 17 lines

The PDF you cited says this:

"Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
in many platforms, including x86-64. In our example, the content of user address 0xBEEF000 is also accessible through kernel address 0xFFFF87FF9F080000 as plain, executable code."

Is this actually true of modern x86-64 kernels? We've locked down W^X protections in general.

I.e. this conclusion:

"Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
triggering the kernel to dereference it, an attacker can directly execute shell code with kernel privileges."

Quoted 6 lines

... appears to be predicated on imperfect W^X protections on the x86-64 kernel.

Do such holes exist on the latest x86-64 kernel? If yes, is there a reason to believe that these W^X holes cannot be fixed, or that any fix would be more expensive than XPFO?

Even if physmap is not executable, return-oriented programming (ROP) can still be used to launch an attack. Instead of placing executable code at user address 0xBEEF000, attacker can place an ROP payload there. kfptr is then overwritten to point to a stack-pivoting gadget. Using the physmap address aliasing, the ROP payload becomes kernel-mode stack. The execution can then be hijacked upon execution of ret instruction. This is a gist of the subsection titled "Non-executable physmap" under section 6.2 and it looked convincing enough to me. If you have a different take on this, I am very interested in your point of view.

I also proposed using a gcc plugin (or upstream gcc feature) to add some instrumentation to any code that pops RSP to verify that the resulting (unsigned) change in RSP is between 0 and THREAD_SIZE bytes. This will make ROP quite a bit harder.

Quoted 51 lines · Khalid Aziz

Quoted 17 lines · Ingo Molnar

Quoted 6 lines · Khalid Aziz

Quoted 5 lines

I.e. the original motivation of the XPFO patches was to prevent execution of direct kernel mappings. Is this motivation still present if those mappings are non-executable?

(Sorry if this has been asked and answered in previous discussions.)

Quoted 7 lines

Hi Ingo,

That is a good question. Because of the cost of XPFO, we have to be very sure we need this protection. The paper from Vasileios, Michalis and Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1 and 6.2.

Quoted 4 lines

So it would be nice if you could generally summarize external arguments when defending a patchset, instead of me having to dig through a PDF which not only causes me to spend time that you probably already spent reading that PDF, but I might also interpret it incorrectly. ;-)

Sorry, you are right. Even though that paper explains it well, a summary is always useful.

Quoted 17 lines

The PDF you cited says this:

"Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
in many platforms, including x86-64. In our example, the content of user address 0xBEEF000 is also accessible through kernel address 0xFFFF87FF9F080000 as plain, executable code."

Is this actually true of modern x86-64 kernels? We've locked down W^X protections in general.

I.e. this conclusion:

"Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
triggering the kernel to dereference it, an attacker can directly execute shell code with kernel privileges."

Quoted 6 lines

... appears to be predicated on imperfect W^X protections on the x86-64 kernel.

Do such holes exist on the latest x86-64 kernel? If yes, is there a reason to believe that these W^X holes cannot be fixed, or that any fix would be more expensive than XPFO?

Quoted 9 lines

Even if physmap is not executable, return-oriented programming (ROP) can still be used to launch an attack. Instead of placing executable code at user address 0xBEEF000, attacker can place an ROP payload there. kfptr is then overwritten to point to a stack-pivoting gadget. Using the physmap address aliasing, the ROP payload becomes kernel-mode stack. The execution can then be hijacked upon execution of ret instruction. This is a gist of the subsection titled "Non-executable physmap" under section 6.2 and it looked convincing enough to me. If you have a different take on this, I am very interested in your point of view.

My issue with all this is that XPFO is really very expensive. I think that, if we're going to seriously consider upstreaming expensive exploit mitigations like this, we should consider others first, in particular CFI techniques. grsecurity's RAP would be a great start. I also proposed using a gcc plugin (or upstream gcc feature) to add some instrumentation to any code that pops RSP to verify that the resulting (unsigned) change in RSP is between 0 and THREAD_SIZE bytes. This will make ROP quite a bit harder.

index 858b6c0b9a15..9b36da94760e 100644

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

@@ -2997,6 +2997,12 @@
 

      nox2apic    [X86-64,APIC] Do not enable x2APIC mode.

+    noxpfo        [XPFO] Disable eXclusive Page Frame Ownership (XPFO) +            when CONFIG_XPFO is on. Physical pages mapped into +            user applications will also be mapped in the
+            kernel's address space as if CONFIG_XPFO was not +            enabled.
+
      cpu0_hotplug    [X86] Turn on CPU0 hotplug feature when CONFIG_BO OTPARAM_HOTPLUG_CPU0 is off.
              Some features depend on CPU0. Known dependencies are:

Review

Quoted 1 line · Khalid Aziz

(quoted diff) +0 -0

b/Documentation/admin-guide/kernel-parameters.txt

Quoted 7 lines

index 858b6c0b9a15..9b36da94760e 100644

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

@@ -2997,6 +2997,12 @@
 

      nox2apic    [X86-64,APIC] Do not enable x2APIC mode.

Quoted 9 lines

+    noxpfo        [XPFO] Disable eXclusive Page Frame Ownership (XPFO) +            when CONFIG_XPFO is on. Physical pages mapped into +            user applications will also be mapped in the
+            kernel's address space as if CONFIG_XPFO was not +            enabled.
+
      cpu0_hotplug    [X86] Turn on CPU0 hotplug feature when CONFIG_BO OTPARAM_HOTPLUG_CPU0 is off.
              Some features depend on CPU0. Known dependencies are:

Given the big performance impact that XPFO can have. It should be off by default when configured. Instead, the xpfo option should be used to enable it.

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

index 2779ace16d23..5c0e1581fa56 100644
@@ -1437,6 +1437,32 @@ static inline bool arch_has_pfn_modify_check(void)
 	return boot_cpu_has_bug(X86_BUG_L1TF);
 }
 
+/*
+ * The current flushing context - we pass it instead of 5 arguments:
+ */
+struct cpa_data {
+	unsigned long	*vaddr;
+	pgd_t		*pgd;
+	pgprot_t	mask_set;
+	pgprot_t	mask_clr;
+	unsigned long	numpages;
+	unsigned long	curpage;
+	unsigned long	pfn;
+	unsigned int	flags;
+	unsigned int	force_split		: 1,
+			force_static_prot	: 1;
+	struct page	**pages;
+};

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

 #include <asm-generic/pgtable.h>
 #endif	/* __ASSEMBLY__ */
 

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

new file mode 100644
index 000000000000..3045bb7e4659
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 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 <vpk-9N4TdPD67tT2fBVCVOL8/[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.
+ */
+
+#include <linux/mm.h>
+
+#include <asm/tlbflush.h>
+
+extern spinlock_t cpa_lock;
+
+/* Update a single kernel page table entry */
+inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
+{
+	unsigned int level;
+	pgprot_t msk_clr;
+	pte_t *pte = lookup_address((unsigned long)kaddr, &level);

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

+		break;
+	case PG_LEVEL_2M:
+	case PG_LEVEL_1G: {
+		struct cpa_data cpa = { };
+		int do_split;
+
+		if (level == PG_LEVEL_2M)
+			msk_clr = pmd_pgprot(*(pmd_t *)pte);
+		else
+			msk_clr = pud_pgprot(*(pud_t *)pte);
+
+		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/mm/xpfo.c new file +5 -0

+				break;
+			}
+
+			if (!debug_pagealloc_enabled())
+				spin_lock(&cpa_lock);

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

+	default:
+		WARN(1, "xpfo: unsupported page level %x\n", level);
+	}
+
+}
+EXPORT_SYMBOL_GPL(set_kpte);
+
+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;

Quoted 28 lines · Khalid Aziz

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

index 2779ace16d23..5c0e1581fa56 100644
@@ -1437,6 +1437,32 @@ static inline bool arch_has_pfn_modify_check(void)
 	return boot_cpu_has_bug(X86_BUG_L1TF);
 }
 
+/*
+ * The current flushing context - we pass it instead of 5 arguments:
+ */
+struct cpa_data {
+	unsigned long	*vaddr;
+	pgd_t		*pgd;
+	pgprot_t	mask_set;
+	pgprot_t	mask_clr;
+	unsigned long	numpages;
+	unsigned long	curpage;
+	unsigned long	pfn;
+	unsigned int	flags;
+	unsigned int	force_split		: 1,
+			force_static_prot	: 1;
+	struct page	**pages;
+};
+
+
+int
+should_split_large_page(pte_t *kpte, unsigned long address,

arch/x86/include/asm/pgtable.h static inline bool arch_has_pfn_modify_check(void) · line 1465

Quoted 6 lines

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

+			struct cpa_data *cpa);
+extern spinlock_t cpa_lock;
+int
+__split_large_page(struct cpa_data *cpa, pte_t *kpte, unsigned long address,
+		   struct page *base);
+

I really hate exposing all that.

Quoted 41 lines

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

 #include <asm-generic/pgtable.h>
 #endif	/* __ASSEMBLY__ */
 

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

new file mode 100644
index 000000000000..3045bb7e4659
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 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 <vpk-9N4TdPD67tT2fBVCVOL8/[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.
+ */
+
+#include <linux/mm.h>
+
+#include <asm/tlbflush.h>
+
+extern spinlock_t cpa_lock;
+
+/* Update a single kernel page table entry */
+inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
+{
+	unsigned int level;
+	pgprot_t msk_clr;
+	pte_t *pte = lookup_address((unsigned long)kaddr, &level);
+
+	if (unlikely(!pte)) {
+		WARN(1, "xpfo: invalid address %p\n", kaddr);
+		return;

arch/x86/mm/xpfo.c line 36

Quoted 6 lines

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

+	}
+
+	switch (level) {
+	case PG_LEVEL_4K:
+		set_pte_atomic(pte, pfn_pte(page_to_pfn(page),
+			       canon_pgprot(prot)));

(sorry, do we also need a nikon_pgprot() ? :-)

Quoted 25 lines

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

+		break;
+	case PG_LEVEL_2M:
+	case PG_LEVEL_1G: {
+		struct cpa_data cpa = { };
+		int do_split;
+
+		if (level == PG_LEVEL_2M)
+			msk_clr = pmd_pgprot(*(pmd_t *)pte);
+		else
+			msk_clr = pud_pgprot(*(pud_t *)pte);
+
+		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;
+
+
+		do_split = should_split_large_page(pte, (unsigned long)kaddr,
+						   &cpa);
+		if (do_split) {

arch/x86/mm/xpfo.c line 65

Quoted 5 lines

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

+			struct page *base;
+
+			base = alloc_pages(GFP_ATOMIC, 0);
+			if (!base) {
+				WARN(1, "xpfo: failed to split large page\n");

You have to be fcking kidding right? A WARN when a GFP_ATOMIC allocation fails?!

Quoted 9 lines

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

+				break;
+			}
+
+			if (!debug_pagealloc_enabled())
+				spin_lock(&cpa_lock);
+			if  (__split_large_page(&cpa, pte, (unsigned long)kaddr,
+						base) < 0) {
+				__free_page(base);
+				WARN(1, "xpfo: failed to split large page\n");

arch/x86/mm/xpfo.c line 80

Quoted 6 lines

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

+			}
+			if (!debug_pagealloc_enabled())
+				spin_unlock(&cpa_lock);
+		}
+
+		break;

Ever heard of helper functions?

arch/x86/mm/xpfo.c line 85

Quoted 6 lines

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

+	}
+	case PG_LEVEL_512G:
+		/* fallthrough, splitting infrastructure doesn't
+		 * support 512G pages.
+		 */

Broken coment style.

Quoted 32 lines

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

+	default:
+		WARN(1, "xpfo: unsupported page level %x\n", level);
+	}
+
+}
+EXPORT_SYMBOL_GPL(set_kpte);
+
+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;
+	case PG_LEVEL_1G:
+		size = PUD_SIZE;
+		break;
+	default:

arch/x86/mm/xpfo.c line 122

Quoted 6 lines

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

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

You call this from IRQ/IRQ-disabled context... that _CANNOT_ be right.

From: Julian Stecklina <[email protected]>

Only the xpfo_kunmap call that needs to actually unmap the page needs to be serialized. We need to be careful to handle the case, where after the atomic decrement of the mapcount, a xpfo_kmap increased the mapcount again. In this case, we can safely skip modifying the page table.

Model-checked with up to 4 concurrent callers with Spin.

Signed-off-by Julian Stecklina <[email protected]>

Signed-off-by Khalid Aziz <[email protected]>

Cc Khalid Aziz <[email protected]>

Cc [email protected]

Cc [email protected]

Cc Vasileios P. Kemerlis <[email protected]>

Cc Juerg Haefliger <[email protected]>

Cc Tycho Andersen <[email protected]>

Cc Marco Benatto <[email protected]>

Cc David Woodhouse <[email protected]>

---
 include/linux/xpfo.h | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

include/linux/xpfo.h +9 -8

index 2318c7eb5fb7..37e7f52fa6ce 100644
@@ -61,6 +61,7 @@ static inline void xpfo_kmap(void *kaddr, struct page *page)
 static inline void xpfo_kunmap(void *kaddr, struct page *page)
 {
 	unsigned long flags;
+	bool flush_tlb = false;
 
 	if (!static_branch_unlikely(&xpfo_inited))
 		return;

@@ -72,18 +73,23 @@ static inline void xpfo_kunmap(void *kaddr, struct page *page)
 	 * The page is to be allocated back to user space, so unmap it from
 	 * the kernel, flush the TLB and tag it as a user page.
 	 */
-	spin_lock_irqsave(&page->xpfo_lock, flags);
-
 	if (atomic_dec_return(&page->xpfo_mapcount) == 0) {
-#ifdef CONFIG_XPFO_DEBUG
-		WARN_ON(PageXpfoUnmapped(page));
-#endif
-		SetPageXpfoUnmapped(page);
-		set_kpte(kaddr, page, __pgprot(0));
-		xpfo_flush_kernel_tlb(page, 0);
+		spin_lock_irqsave(&page->xpfo_lock, flags);
+
+		/*
+		 * In the case, where we raced with kmap after the
+		 * atomic_dec_return, we must not nuke the mapping.
+		 */
+		if (atomic_read(&page->xpfo_mapcount) == 0) {
+			SetPageXpfoUnmapped(page);

Quoted 63 lines · Khalid Aziz

From: Julian Stecklina <[email protected]>

Only the xpfo_kunmap call that needs to actually unmap the page needs to be serialized. We need to be careful to handle the case, where after the atomic decrement of the mapcount, a xpfo_kmap increased the mapcount again. In this case, we can safely skip modifying the page table.

Model-checked with up to 4 concurrent callers with Spin.

Signed-off-by Julian Stecklina <[email protected]>

Signed-off-by Khalid Aziz <[email protected]>

Cc Khalid Aziz <[email protected]>

Cc [email protected]

Cc [email protected]

Cc Vasileios P. Kemerlis <[email protected]>

Cc Juerg Haefliger <[email protected]>

Cc Tycho Andersen <[email protected]>

Cc Marco Benatto <[email protected]>

Cc David Woodhouse <[email protected]>

---
 include/linux/xpfo.h | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

include/linux/xpfo.h +13 -8

index 2318c7eb5fb7..37e7f52fa6ce 100644
@@ -61,6 +61,7 @@ static inline void xpfo_kmap(void *kaddr, struct page *page)
 static inline void xpfo_kunmap(void *kaddr, struct page *page)
 {
 	unsigned long flags;
+	bool flush_tlb = false;
 
 	if (!static_branch_unlikely(&xpfo_inited))
 		return;

@@ -72,18 +73,23 @@ static inline void xpfo_kunmap(void *kaddr, struct page *page)
 	 * The page is to be allocated back to user space, so unmap it from
 	 * the kernel, flush the TLB and tag it as a user page.
 	 */
-	spin_lock_irqsave(&page->xpfo_lock, flags);
-
 	if (atomic_dec_return(&page->xpfo_mapcount) == 0) {
-#ifdef CONFIG_XPFO_DEBUG
-		WARN_ON(PageXpfoUnmapped(page));
-#endif
-		SetPageXpfoUnmapped(page);
-		set_kpte(kaddr, page, __pgprot(0));
-		xpfo_flush_kernel_tlb(page, 0);
+		spin_lock_irqsave(&page->xpfo_lock, flags);
+
+		/*
+		 * In the case, where we raced with kmap after the
+		 * atomic_dec_return, we must not nuke the mapping.
+		 */
+		if (atomic_read(&page->xpfo_mapcount) == 0) {
+			SetPageXpfoUnmapped(page);
+			set_kpte(kaddr, page, __pgprot(0));
+			flush_tlb = true;
+		}
+		spin_unlock_irqrestore(&page->xpfo_lock, flags);
 	}
 

include/linux/xpfo.h static inline void xpfo_kunmap(void *kaddr, struct page *page) · line 93

Quoted 4 lines

include/linux/xpfo.h +2 -1

-	spin_unlock_irqrestore(&page->xpfo_lock, flags);
+	if (flush_tlb)
+		xpfo_flush_kernel_tlb(page, 0);
 }

This doesn't help with the TLB invalidation issue, AFAICT this is still completely buggered. kunmap_atomic() can be called from IRQ context.

XPFO flushes kernel space TLB entries for pages that are now mapped in userspace on not only the current CPU but also all other CPUs synchronously. Processes on each core allocating pages causes a flood of IPI messages to all other cores to flush TLB entries. Many of these messages are to flush the entire TLB on the core if the number of entries being flushed from local core exceeds tlb_single_page_flush_ceiling. The cost of TLB flush caused by unmapping pages from physmap goes up dramatically on machines with high core count.

This patch flushes relevant TLB entries for current process or entire TLB depending upon number of entries for the current CPU and posts a pending TLB flush on all other CPUs when a page is unmapped from kernel space and mapped in userspace. Each core checks the pending TLB flush flag for itself on every context switch, flushes its TLB if the flag is set and clears it. This patch potentially aggregates multiple TLB flushes into one. This has very significant impact especially on machines with large core counts.

Quoted 11 lines · Khalid Aziz

XPFO flushes kernel space TLB entries for pages that are now mapped in userspace on not only the current CPU but also all other CPUs synchronously. Processes on each core allocating pages causes a flood of IPI messages to all other cores to flush TLB entries. Many of these messages are to flush the entire TLB on the core if the number of entries being flushed from local core exceeds tlb_single_page_flush_ceiling. The cost of TLB flush caused by unmapping pages from physmap goes up dramatically on machines with high core count.

Quoted 9 lines

This patch flushes relevant TLB entries for current process or entire TLB depending upon number of entries for the current CPU and posts a pending TLB flush on all other CPUs when a page is unmapped from kernel space and mapped in userspace. Each core checks the pending TLB flush flag for itself on every context switch, flushes its TLB if the flag is set and clears it. This patch potentially aggregates multiple TLB flushes into one. This has very significant impact especially on machines with large core counts.

Why is this a reasonable strategy?

Quoted 4 lines

arch/x86/mm/tlb.c +3 -0

+void xpfo_flush_tlb_kernel_range(unsigned long start, unsigned long end)
+{
+       struct cpumask tmp_mask;

arch/x86/mm/tlb.c void flush_tlb_kernel_range(unsigned long start, unsigned long end) · line 842

Quoted 10 lines

arch/x86/mm/tlb.c +10 -0

+
+       /*
+        * Balance as user space task's flush, a bit conservative.
+        * Do a local flush immediately and post a pending flush on all
+        * other CPUs. Local flush can be a range flush or full flush
+        * depending upon the number of entries to be flushed. Remote
+        * flushes will be done by individual processors at the time of
+        * context switch and this allows multiple flush requests from
+        * other CPUs to be batched together.
+        */

I don't like this function at all. A core function like this is a contract of sorts between the caller and the implementation. There is no such thing as an "xpfo" flush, and this function's behavior isn't at all well defined. For flush_tlb_kernel_range(), I can tell you exactly what that function does, and the implementation is either correct or incorrect. With this function, I have no idea what is actually required, and I can't possibly tell whether it's correct.

As far as I can see, xpfo_flush_tlb_kernel_range() actually means "flush this range on this CPU right now, and flush it on remote CPUs eventually". It would be valid, but probably silly, to flush locally and to never flush at all on remote CPUs. This makes me wonder what the point is.

When xpfo unmaps a page from physmap only (after mapping the page in userspace in response to an allocation request from userspace) on one processor, there is a small window of opportunity for ret2dir attack on other cpus until the TLB entry in physmap for the unmapped pages on other cpus is cleared. Forcing that to happen synchronously is the expensive part. A multiple of these requests can come in over a very short time across multiple processors resulting in every cpu asking every other cpusto flush TLB just to close this small window of vulnerability in the kernel. If each request is processed synchronously, each CPU will do multiple TLB flushes in short order. If we could consolidate these TLB flush requests instead and do one TLB flush on each cpu at the time of context switch, we can reduce the performance impact significantly. This bears out in real life measuring the system time when doing a parallel kernel build on a large server. Without this, system time on 96-core server when doing "make -j60 all" went up 26x. After this optimization, impact went down to 1.44x.

The trade-off with this strategy is, the kernel on a cpu is vulnerable for a short time if the current running processor is the malicious

process. Is that an acceptable trade-off?

Quoted 1 line

process. Is that an acceptable trade-off?

You are not seriously asking whether creating a user controllable ret2dir attack window is a acceptable trade-off? April 1st was a few days ago.

Quoted 3 lines · Thomas Gleixner

Quoted 1 line

process. Is that an acceptable trade-off?

You are not seriously asking whether creating a user controllable ret2dir attack window is a acceptable trade-off? April 1st was a few days ago.

Well, let's not forget that this set at least takes us from "always vulnerable to ret2dir" to a choice between:

1. fast-ish and "vulnerable to ret2dir for a user-controllable window"
2. slow and "mitigated against ret2dir"

Sounds like we need a mechanism that will do the deferred XPFO TLB flushes whenever the kernel is entered, and not _just_ at context switch time. This permits an app to run in userspace with stale kernel TLB entries as long as it wants... that's harmless.

Quoted 12 lines · Dave Hansen

Quoted 3 lines · Thomas Gleixner

Quoted 1 line

process. Is that an acceptable trade-off?

You are not seriously asking whether creating a user controllable ret2dir attack window is a acceptable trade-off? April 1st was a few days ago.

Well, let's not forget that this set at least takes us from "always vulnerable to ret2dir" to a choice between:

Quoted 7 lines

1. fast-ish and "vulnerable to ret2dir for a user-controllable window"
2. slow and "mitigated against ret2dir"

Sounds like we need a mechanism that will do the deferred XPFO TLB flushes whenever the kernel is entered, and not _just_ at context switch time. This permits an app to run in userspace with stale kernel TLB entries as long as it wants... that's harmless.

I don’t think this is good enough. The bad guys can enter the kernel and arrange for the kernel to wait, *in kernel*, for long enough to set up the attack. userfaultfd is the most obvious way, but there are plenty. I suppose we could do the flush at context switch *and* entry. I bet that performance still utterly sucks, though — on many workloads, this turns every entry into a full flush, and we already know exactly how much that sucks — it’s identical to KPTI without PCID. (And yes, if we go this route, we need to merge this logic together — we shouldn’t write CR3 twice on entry).

I feel like this whole approach is misguided. ret2dir is not such a game changer that fixing it is worth huge slowdowns. I think all this effort should be spent on some kind of sensible CFI. For example, we should be able to mostly squash ret2anything by inserting a check that the high bits of RSP match the value on the top of the stack before any code that pops RSP. On an FPO build, there aren’t all that many hot POP RSP instructions, I think.

(Actually, checking the bits is suboptimal. Do:

unsigned long offset = *rsp - rsp;
offset >>= THREAD_SHIFT;
if (unlikely(offset))
BUG();
POP RSP;

This means that it’s also impossible to trick a function to return into a buffer that is on that function’s stack.)

Quoted 26 lines · Andy Lutomirski

Quoted 12 lines · Dave Hansen

Quoted 3 lines · Thomas Gleixner

Quoted 1 line

process. Is that an acceptable trade-off?

You are not seriously asking whether creating a user controllable ret2dir attack window is a acceptable trade-off? April 1st was a few days ago.

Well, let's not forget that this set at least takes us from "always vulnerable to ret2dir" to a choice between:

Quoted 7 lines

1. fast-ish and "vulnerable to ret2dir for a user-controllable window"
2. slow and "mitigated against ret2dir"

Sounds like we need a mechanism that will do the deferred XPFO TLB flushes whenever the kernel is entered, and not _just_ at context switch time. This permits an app to run in userspace with stale kernel TLB entries as long as it wants... that's harmless.

I don’t think this is good enough. The bad guys can enter the kernel and arrange for the kernel to wait, *in kernel*, for long enough to set up the attack. userfaultfd is the most obvious way, but there are plenty. I suppose we could do the flush at context switch *and* entry. I bet that performance still utterly sucks, though — on many workloads, this turns every entry into a full flush, and we already know exactly how much that sucks — it’s identical to KPTI without PCID. (And yes, if we go this route, we need to merge this logic together — we shouldn’t write CR3 twice on entry).

I feel like this whole approach is misguided. ret2dir is not such a game changer that fixing it is worth huge slowdowns. I think all this effort should be spent on some kind of sensible CFI. For example, we should be able to mostly squash ret2anything by inserting a check that the high bits of RSP match the value on the top of the stack before any code that pops RSP. On an FPO build, there aren’t all that many hot POP RSP instructions, I think.

Quoted 7 lines

(Actually, checking the bits is suboptimal. Do:

unsigned long offset = *rsp - rsp;
offset >>= THREAD_SHIFT;
if (unlikely(offset))
BUG();
POP RSP;

This is a neat trick, and definitely prevents going random places in the heap. But,

Quoted 1 line

This means that it’s also impossible to trick a function to return into a buffer that is on that function’s stack.)

Why is this true? All you're checking is that you can't shift the "location" of the stack. If you can inject stuff into a stack buffer, can't you just inject the right frame to return to your code as well, so you don't have to shift locations?

Tycho

Quoted 15 lines · Dave Hansen

Quoted 3 lines · Thomas Gleixner

Quoted 1 line

process. Is that an acceptable trade-off?

You are not seriously asking whether creating a user controllable ret2dir attack window is a acceptable trade-off? April 1st was a few days ago.

Well, let's not forget that this set at least takes us from "always vulnerable to ret2dir" to a choice between:

1. fast-ish and "vulnerable to ret2dir for a user-controllable window"
2. slow and "mitigated against ret2dir"

Quoted 4 lines

Sounds like we need a mechanism that will do the deferred XPFO TLB flushes whenever the kernel is entered, and not _just_ at context switch time. This permits an app to run in userspace with stale kernel TLB entries as long as it wants... that's harmless.

I don’t think this is good enough. The bad guys can enter the kernel and arrange for the kernel to wait, *in kernel*, for long enough to set up the attack. userfaultfd is the most obvious way, but there are plenty. I suppose we could do the flush at context switch *and* entry. I bet that performance still utterly sucks, though — on many workloads, this turns every entry into a full flush, and we already know exactly how much that sucks — it’s identical to KPTI without PCID. (And yes, if we go this route, we need to merge this logic together — we shouldn’t write CR3 twice on entry).

I feel like this whole approach is misguided. ret2dir is not such a game changer that fixing it is worth huge slowdowns. I think all this effort should be spent on some kind of sensible CFI. For example, we should be able to mostly squash ret2anything by inserting a check that the high bits of RSP match the value on the top of the stack before any code that pops RSP. On an FPO build, there aren’t all that many hot POP RSP instructions, I think.

(Actually, checking the bits is suboptimal. Do:

unsigned long offset = *rsp - rsp;
offset >>= THREAD_SHIFT;
if (unlikely(offset))
BUG();
POP RSP;

This means that it’s also impossible to trick a function to return into a buffer that is on that function’s stack.)

In other words, I think that ret2dir is an insufficient justification for XPFO.

Quoted 4 lines

Sounds like we need a mechanism that will do the deferred XPFO TLB flushes whenever the kernel is entered, and not _just_ at context switch time. This permits an app to run in userspace with stale kernel TLB entries as long as it wants... that's harmless.

I suppose we could do the flush at context switch *and* entry. I bet that performance still utterly sucks, though — on many workloads, this turns every entry into a full flush, and we already know exactly how much that sucks — it’s identical to KPTI without PCID. (And yes, if we go this route, we need to merge this logic together — we shouldn’t write CR3 twice on entry).

In other words, I think that ret2dir is an insufficient justification for XPFO.

Quoted 4 lines · Andy Lutomirski

Quoted 4 lines

Sounds like we need a mechanism that will do the deferred XPFO TLB flushes whenever the kernel is entered, and not _just_ at context switch time. This permits an app to run in userspace with stale kernel TLB entries as long as it wants... that's harmless.

...

Quoted 6 lines

I suppose we could do the flush at context switch *and* entry. I bet that performance still utterly sucks, though — on many workloads, this turns every entry into a full flush, and we already know exactly how much that sucks — it’s identical to KPTI without PCID. (And yes, if we go this route, we need to merge this logic together — we shouldn’t write CR3 twice on entry).

Yeah, probably true.

Just eyeballing this, it would mean mapping the "cpu needs deferred flush" variable into the cpu_entry_area, which doesn't seem too awful.

I think the basic overall concern is that the deferred flush leaves too many holes and by the time we close them sufficiently, performance will suck again. Seems like a totally valid concern, but my crystal ball is hazy on whether it will be worth it in the end to many folks

...

Quoted 2 lines

In other words, I think that ret2dir is an insufficient justification for XPFO.

Yeah, other things that it is good for have kinda been lost in the noise. I think I first started looking at this long before Meltdown and L1TF were public.

There are hypervisors out there that simply don't (persistently) map user data. They can't leak user data because they don't even have access to it in their virtual address space. Those hypervisors had a much easier time with L1TF mitigation than we did. Basically, they could flush the L1 after user data was accessible instead of before untrusted guest code runs.

My hope is that XPFO could provide us similar protection. But, somebody's got to poke at it for a while to see how far they can push it.

IMNHO, XPFO is *always* going to be painful for kernel compiles. But, cloud providers aren't doing a lot of kernel compiles on their KVM hosts, and they deeply care about leaking their users' data.

At the risk of asking stupid questions: we already have a mechanism for this: highmem. Can we enable highmem on x86_64, maybe with some heuristics to make it work well?

At the risk of asking stupid questions: we already have a mechanism for this: highmem. Can we enable highmem on x86_64, maybe with some heuristics to make it work well?

Quoted 3 lines · Thomas Gleixner

Quoted 1 line

process. Is that an acceptable trade-off?

You are not seriously asking whether creating a user controllable ret2dir attack window is a acceptable trade-off? April 1st was a few days ago.

Well, let's not forget that this set at least takes us from "always vulnerable to ret2dir" to a choice between:

1. fast-ish and "vulnerable to ret2dir for a user-controllable window"
2. slow and "mitigated against ret2dir"

Sounds like we need a mechanism that will do the deferred XPFO TLB flushes whenever the kernel is entered, and not _just_ at context switch time. This permits an app to run in userspace with stale kernel TLB entries as long as it wants... that's harmless.

Quoted 27 lines

Quoted 17 lines · Khalid Aziz

On 4/3/19 10:10 PM, Andy Lutomirski wrote:
On Wed, Apr 3, 2019 at 10:36 AM Khalid Aziz <[email protected]> wrote:

XPFO flushes kernel space TLB entries for pages that are now mapped in userspace on not only the current CPU but also all other CPUs synchronously. Processes on each core allocating pages causes a flood of IPI messages to all other cores to flush TLB entries. Many of these messages are to flush the entire TLB on the core if the number of entries being flushed from local core exceeds tlb_single_page_flush_ceiling. The cost of TLB flush caused by unmapping pages from physmap goes up dramatically on machines with high core count.

Quoted 9 lines

This patch flushes relevant TLB entries for current process or entire TLB depending upon number of entries for the current CPU and posts a pending TLB flush on all other CPUs when a page is unmapped from kernel space and mapped in userspace. Each core checks the pending TLB flush flag for itself on every context switch, flushes its TLB if the flag is set and clears it. This patch potentially aggregates multiple TLB flushes into one. This has very significant impact especially on machines with large core counts.

Quoted 1 line

Why is this a reasonable strategy?

Ideally when pages are unmapped from physmap, all CPUs would be sent IPI synchronously to flush TLB entry for those pages immediately. This may be ideal from correctness and consistency point of view, but it also results in IPI storm and repeated TLB flushes on all processors. Any time a page is allocated to userspace, we are going to go through this and it is very expensive. On a 96-core server, performance degradation is 26x!!

When xpfo unmaps a page from physmap only (after mapping the page in userspace in response to an allocation request from userspace) on one processor, there is a small window of opportunity for ret2dir attack on other cpus until the TLB entry in physmap for the unmapped pages on other cpus is cleared.

arch/x86/mm/tlb.c +14 -0

index 999d6d8f0bef..cc806a01a0eb 100644
@@ -37,6 +37,20 @@
  */
 #define LAST_USER_MM_IBPB	0x1UL
 
+/*
+ * A TLB flush may be needed to flush stale TLB entries
+ * for pages that have been mapped into userspace and unmapped
+ * from kernel space. This TLB flush needs to be propagated to
+ * all CPUs. Asynchronous flush requests to all CPUs can cause
+ * significant performance imapct. Queue a pending flush for
+ * a CPU instead. Multiple of these requests can then be handled
+ * by a CPU at a less disruptive time, like context switch, in
+ * one go and reduce performance impact significantly. Following
+ * data structure is used to keep track of CPUs with pending full
+ * TLB flush forced by xpfo.
+ */
+static cpumask_t pending_xpfo_flush;
+
 /*
  * We get here when we do something requiring a TLB invalidation
  * but could not go invalidate all of the contexts.  We do the

@@ -321,6 +335,16 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 		__flush_tlb_all();
 	}

arch/x86/mm/tlb.c +29 -0

+
 	this_cpu_write(cpu_tlbstate.is_lazy, false);
 
 	/*

@@ -803,6 +827,34 @@ void flush_tlb_kernel_range(unsigned long start, unsigned long end)
 	}
 }
 
+void xpfo_flush_tlb_kernel_range(unsigned long start, unsigned long end)
+{
+	struct cpumask tmp_mask;
+
+	/*
+	 * Balance as user space task's flush, a bit conservative.
+	 * Do a local flush immediately and post a pending flush on all
+	 * other CPUs. Local flush can be a range flush or full flush
+	 * depending upon the number of entries to be flushed. Remote
+	 * flushes will be done by individual processors at the time of
+	 * context switch and this allows multiple flush requests from
+	 * other CPUs to be batched together.
+	 */
+	if (end == TLB_FLUSH_ALL ||
+	    (end - start) > tlb_single_page_flush_ceiling << PAGE_SHIFT) {
+		do_flush_tlb_all(NULL);
+	} else {
+		struct flush_tlb_info info;
+
+		info.start = start;
+		info.end = end;
+		do_kernel_range_flush(&info);
+	}
+	cpumask_setall(&tmp_mask);
+	__cpumask_clear_cpu(smp_processor_id(), &tmp_mask);
+	cpumask_or(&pending_xpfo_flush, &pending_xpfo_flush, &tmp_mask);
+}
+
 void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
 {
 	struct flush_tlb_info info = {

arch/x86/mm/xpfo.c +0 -0

index b42513347865..638eee5b1f09 100644