[PATCH v7 00/14] x86/alternative: text_poke() enhancements

[PATCH v7 00/14] x86/alternative: text_poke() enhancements

[Nadav Amit]

This patch-set addresses some issues that might affect the security and
the correctness of code patching. It was originally small and mainly
intended to remove the text-poking fixmap PTEs, which can cause PTEs
cached in the TLB in remote cores for unbounded time.

It was then suggested by tglx and Andy that patching of modules can be
simpler by making module code non-executable during setup. This opened a
can of worms, since it required changes of kprobe and ftrace. Ftrace, it
turns out, patches the code using a homegrown mechanism, so it made
sense to make it use text_poke_*() instead. And then, module unloading
seemed fragile and susceptible for attacks, so it required some
attention as well.

This whole story is to clarify two points: (a) this patch-set does *not*
have a full threat-model in mind. It does harden security a bit by
shortening the time-window in which writable executable mappings are
set. Since it consolidates kernel code modifications, it may be used as
a basis for some a future code integrity mechanism, a-la Microsoft's
hypervisor enforced code integrity (HVCI). However, this patch-set does
not provide HVCI-like security.

Which leads me to (b) - the patch-set is big "enough" IMHO. Indeed,
there are open security issues in the kernel when it comes to W^X.  But
some people would want to use Andy's temporary mm-struct for other uses.
So additional security hardening may be left for future patches.

v6->v7:
- Fix kprobes breakage [Rick Edgecombe]
- Fix ftrace breakage [Rick Edgecombe]
- Fix module unloading issues (setting X, race with patching)

v5->v6:
- Panic if anything goes wrong when poking [peterZ]

v4->v5:
- Fix Xen breakage [Damian Tometzki]
- BUG_ON() when poking_mm initialization fails [peterZ]
- Better comments on "x86/mm: temporary mm struct"
- Cleaner removal of the custom poker

v3->v4:
- Setting modules as RO when loading [andy, tglx]
- Adding text_poke_kgdb() to keep the text_mutex assertion [tglx]
- Simpler logic to decide when to use early-poking [peterZ]
- More cleanup

v2->v3:
- Remove the fallback path in text_poke() [peterZ]
- poking_init() was broken due to the local variable poking_addr
- Preallocate tables for the temporary-mm to avoid sleep-in-atomic
- Prevent KASAN from yelling at text_poke()

v1->v2:
- Partial revert of 9222f606506c added to 1/6 [masami]
- Added Masami's reviewed-by tag

RFC->v1:
- Added handling of error in get_locked_pte()
- Remove lockdep assertion, clarify text_mutex use instead [masami]
- Comment fix [peterz]
- Removed remainders of text_poke return value [masami]
- Use __weak for poking_init instead of macros [masami]
- Simplify error handling in poking_init [masami]

Andy Lutomirski (1):
  x86/mm: temporary mm struct

Nadav Amit (13):
  Fix "x86/alternatives: Lockdep-enforce text_mutex in text_poke*()"
  x86/jump_label: Use text_poke_early() during early init
  fork: provide a function for copying init_mm
  x86/alternative: initializing temporary mm for patching
  x86/alternative: use temporary mm for text poking
  x86/kgdb: avoid redundant comparison of patched code
  x86/ftrace: Use text_poke_*() infrastructure
  x86/kprobes: Instruction pages initialization enhancements
  x86: avoid W^X being broken during modules loading
  x86/jump-label: remove support for custom poker
  x86/alternative: Remove the return value of text_poke_*()
  module: Do not set nx for module memory before freeing
  module: Prevent module removal racing with text_poke()

 arch/x86/include/asm/fixmap.h        |   2 -
 arch/x86/include/asm/mmu_context.h   |  32 +++++
 arch/x86/include/asm/pgtable.h       |   3 +
 arch/x86/include/asm/text-patching.h |   7 +-
 arch/x86/kernel/alternative.c        | 197 ++++++++++++++++++++-------
 arch/x86/kernel/ftrace.c             |  74 ++++------
 arch/x86/kernel/jump_label.c         |  19 ++-
 arch/x86/kernel/kgdb.c               |  25 +---
 arch/x86/kernel/kprobes/core.c       |  24 +++-
 arch/x86/kernel/module.c             |   2 +-
 arch/x86/mm/init_64.c                |  35 +++++
 arch/x86/xen/mmu_pv.c                |   2 -
 include/linux/filter.h               |   6 +
 include/linux/sched/task.h           |   1 +
 init/main.c                          |   3 +
 kernel/fork.c                        |  24 +++-
 kernel/module.c                      |  50 +++++--
 17 files changed, 349 insertions(+), 157 deletions(-)

-- 
2.17.1

[PATCH v7 01/14] Fix "x86/alternatives: Lockdep-enforce text_mutex in text_poke*()"

[Nadav Amit]

text_mutex is currently expected to be held before text_poke() is
called, but we kgdb does not take the mutex, and instead *supposedly*
ensures the lock is not taken and will not be acquired by any other core
while text_poke() is running.

The reason for the "supposedly" comment is that it is not entirely clear
that this would be the case if gdb_do_roundup is zero.

This patch creates two wrapper functions, text_poke() and
text_poke_kgdb() which do or do not run the lockdep assertion
respectively.

While we are at it, change the return code of text_poke() to something
meaningful. One day, callers might actually respect it and the existing
BUG_ON() when patching fails could be removed. For kgdb, the return
value can actually be used.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 9222f606506c ("x86/alternatives: Lockdep-enforce text_mutex in text_poke*()")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/include/asm/text-patching.h |  1 +
 arch/x86/kernel/alternative.c        | 52 ++++++++++++++++++++--------
 arch/x86/kernel/kgdb.c               | 11 +++---
 3 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/text-patching.h b/arch/x86/include/asm/text-patching.h
index e85ff65c43c3..f8fc8e86cf01 100644
--- a/arch/x86/include/asm/text-patching.h
+++ b/arch/x86/include/asm/text-patching.h
@@ -35,6 +35,7 @@ extern void *text_poke_early(void *addr, const void *opcode, size_t len);
  * inconsistent instruction while you patch.
  */
 extern void *text_poke(void *addr, const void *opcode, size_t len);
+extern void *text_poke_kgdb(void *addr, const void *opcode, size_t len);
 extern int poke_int3_handler(struct pt_regs *regs);
 extern void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler);
 extern int after_bootmem;
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index ebeac487a20c..c6a3a10a2fd5 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -678,18 +678,7 @@ void *__init_or_module text_poke_early(void *addr, const void *opcode,
 	return addr;
 }
 
-/**
- * text_poke - Update instructions on a live kernel
- * @addr: address to modify
- * @opcode: source of the copy
- * @len: length to copy
- *
- * Only atomic text poke/set should be allowed when not doing early patching.
- * It means the size must be writable atomically and the address must be aligned
- * in a way that permits an atomic write. It also makes sure we fit on a single
- * page.
- */
-void *text_poke(void *addr, const void *opcode, size_t len)
+static void *__text_poke(void *addr, const void *opcode, size_t len)
 {
 	unsigned long flags;
 	char *vaddr;
@@ -702,8 +691,6 @@ void *text_poke(void *addr, const void *opcode, size_t len)
 	 */
 	BUG_ON(!after_bootmem);
 
-	lockdep_assert_held(&text_mutex);
-
 	if (!core_kernel_text((unsigned long)addr)) {
 		pages[0] = vmalloc_to_page(addr);
 		pages[1] = vmalloc_to_page(addr + PAGE_SIZE);
@@ -732,6 +719,43 @@ void *text_poke(void *addr, const void *opcode, size_t len)
 	return addr;
 }
 
+/**
+ * text_poke - Update instructions on a live kernel
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing early patching.
+ * It means the size must be writable atomically and the address must be aligned
+ * in a way that permits an atomic write. It also makes sure we fit on a single
+ * page.
+ */
+void *text_poke(void *addr, const void *opcode, size_t len)
+{
+	lockdep_assert_held(&text_mutex);
+
+	return __text_poke(addr, opcode, len);
+}
+
+/**
+ * text_poke_kgdb - Update instructions on a live kernel by kgdb
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * Only atomic text poke/set should be allowed when not doing early patching.
+ * It means the size must be writable atomically and the address must be aligned
+ * in a way that permits an atomic write. It also makes sure we fit on a single
+ * page.
+ *
+ * Context: should only be used by kgdb, which ensures no other core is running,
+ *	    despite the fact it does not hold the text_mutex.
+ */
+void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
+{
+	return __text_poke(addr, opcode, len);
+}
+
 static void do_sync_core(void *info)
 {
 	sync_core();
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index 8e36f249646e..2636ca8394bd 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -763,13 +763,13 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 	if (!err)
 		return err;
 	/*
-	 * It is safe to call text_poke() because normal kernel execution
+	 * It is safe to call text_poke_kgdb() because normal kernel execution
 	 * is stopped on all cores, so long as the text_mutex is not locked.
 	 */
 	if (mutex_is_locked(&text_mutex))
 		return -EBUSY;
-	text_poke((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
-		  BREAK_INSTR_SIZE);
+	text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
+		       BREAK_INSTR_SIZE);
 	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
 	if (err)
 		return err;
@@ -788,12 +788,13 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 	if (bpt->type != BP_POKE_BREAKPOINT)
 		goto knl_write;
 	/*
-	 * It is safe to call text_poke() because normal kernel execution
+	 * It is safe to call text_poke_kgdb() because normal kernel execution
 	 * is stopped on all cores, so long as the text_mutex is not locked.
 	 */
 	if (mutex_is_locked(&text_mutex))
 		goto knl_write;
-	text_poke((void *)bpt->bpt_addr, bpt->saved_instr, BREAK_INSTR_SIZE);
+	text_poke_kgdb((void *)bpt->bpt_addr, bpt->saved_instr,
+		       BREAK_INSTR_SIZE);
 	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
 	if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))
 		goto knl_write;
-- 
2.17.1

[PATCH v7 02/14] x86/jump_label: Use text_poke_early() during early init

[Nadav Amit]

There is no apparent reason not to use text_poke_early() while we are
during early-init and we do not patch code that might be on the stack
(i.e., we'll return to the middle of the patched code). This appears to
be the case of jump-labels, so do so.

This is required for the next patches that would set a temporary mm for
patching, which is initialized after some static-keys are
enabled/disabled.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/kernel/jump_label.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index aac0c1f7e354..ed5fe274a7d8 100644
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -52,7 +52,12 @@ static void __ref __jump_label_transform(struct jump_entry *entry,
 	jmp.offset = jump_entry_target(entry) -
 		     (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
 
-	if (early_boot_irqs_disabled)
+	/*
+	 * As long as we're UP and not yet marked RO, we can use
+	 * text_poke_early; SYSTEM_BOOTING guarantees both, as we switch to
+	 * SYSTEM_SCHEDULING before going either.
+	 */
+	if (system_state == SYSTEM_BOOTING)
 		poker = text_poke_early;
 
 	if (type == JUMP_LABEL_JMP) {
-- 
2.17.1

[PATCH v7 03/14] x86/mm: temporary mm struct

[Nadav Amit]

From: Andy Lutomirski <luto@kernel.org>

Sometimes we want to set a temporary page-table entries (PTEs) in one of
the cores, without allowing other cores to use - even speculatively -
these mappings. There are two benefits for doing so:

(1) Security: if sensitive PTEs are set, temporary mm prevents their use
in other cores. This hardens the security as it prevents exploding a
dangling pointer to overwrite sensitive data using the sensitive PTE.

(2) Avoiding TLB shootdowns: the PTEs do not need to be flushed in
remote page-tables.

To do so a temporary mm_struct can be used. Mappings which are private
for this mm can be set in the userspace part of the address-space.
During the whole time in which the temporary mm is loaded, interrupts
must be disabled.

The first use-case for temporary PTEs, which will follow, is for poking
the kernel text.

[ Commit message was written by Nadav ]

Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/include/asm/mmu_context.h | 32 ++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 0ca50611e8ce..0141b7fa6d01 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -338,4 +338,36 @@ static inline unsigned long __get_current_cr3_fast(void)
 	return cr3;
 }
 
+typedef struct {
+	struct mm_struct *prev;
+} temporary_mm_state_t;
+
+/*
+ * Using a temporary mm allows to set temporary mappings that are not accessible
+ * by other cores. Such mappings are needed to perform sensitive memory writes
+ * that override the kernel memory protections (e.g., W^X), without exposing the
+ * temporary page-table mappings that are required for these write operations to
+ * other cores.
+ *
+ * Context: The temporary mm needs to be used exclusively by a single core. To
+ *          harden security IRQs must be disabled while the temporary mm is
+ *          loaded, thereby preventing interrupt handler bugs from override the
+ *          kernel memory protection.
+ */
+static inline temporary_mm_state_t use_temporary_mm(struct mm_struct *mm)
+{
+	temporary_mm_state_t state;
+
+	lockdep_assert_irqs_disabled();
+	state.prev = this_cpu_read(cpu_tlbstate.loaded_mm);
+	switch_mm_irqs_off(NULL, mm, current);
+	return state;
+}
+
+static inline void unuse_temporary_mm(temporary_mm_state_t prev)
+{
+	lockdep_assert_irqs_disabled();
+	switch_mm_irqs_off(NULL, prev.prev, current);
+}
+
 #endif /* _ASM_X86_MMU_CONTEXT_H */
-- 
2.17.1

[PATCH v7 04/14] fork: provide a function for copying init_mm

[Nadav Amit]

Provide a function for copying init_mm. This function will be later used
for setting a temporary mm.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 include/linux/sched/task.h |  1 +
 kernel/fork.c              | 24 ++++++++++++++++++------
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index 108ede99e533..ac0a675678f5 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -74,6 +74,7 @@ extern void exit_itimers(struct signal_struct *);
 extern long _do_fork(unsigned long, unsigned long, unsigned long, int __user *, int __user *, unsigned long);
 extern long do_fork(unsigned long, unsigned long, unsigned long, int __user *, int __user *);
 struct task_struct *fork_idle(int);
+struct mm_struct *copy_init_mm(void);
 extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags);
 extern long kernel_wait4(pid_t, int __user *, int, struct rusage *);
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 07cddff89c7b..01d3f5b39363 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1297,13 +1297,20 @@ void mm_release(struct task_struct *tsk, struct mm_struct *mm)
 		complete_vfork_done(tsk);
 }
 
-/*
- * Allocate a new mm structure and copy contents from the
- * mm structure of the passed in task structure.
+/**
+ * dup_mm() - duplicates an existing mm structure
+ * @tsk: the task_struct with which the new mm will be associated.
+ * @oldmm: the mm to duplicate.
+ *
+ * Allocates a new mm structure and copy contents from the provided
+ * @oldmm structure.
+ *
+ * Return: the duplicated mm or NULL on failure.
  */
-static struct mm_struct *dup_mm(struct task_struct *tsk)
+static struct mm_struct *dup_mm(struct task_struct *tsk,
+				struct mm_struct *oldmm)
 {
-	struct mm_struct *mm, *oldmm = current->mm;
+	struct mm_struct *mm;
 	int err;
 
 	mm = allocate_mm();
@@ -1370,7 +1377,7 @@ static int copy_mm(unsigned long clone_flags, struct task_struct *tsk)
 	}
 
 	retval = -ENOMEM;
-	mm = dup_mm(tsk);
+	mm = dup_mm(tsk, current->mm);
 	if (!mm)
 		goto fail_nomem;
 
@@ -2176,6 +2183,11 @@ struct task_struct *fork_idle(int cpu)
 	return task;
 }
 
+struct mm_struct *copy_init_mm(void)
+{
+	return dup_mm(NULL, &init_mm);
+}
+
 /*
  *  Ok, this is the main fork-routine.
  *
-- 
2.17.1

[PATCH v7 05/14] x86/alternative: initializing temporary mm for patching

[Nadav Amit]

To prevent improper use of the PTEs that are used for text patching, we
want to use a temporary mm struct. We initailize it by copying the init
mm.

The address that will be used for patching is taken from the lower area
that is usually used for the task memory. Doing so prevents the need to
frequently synchronize the temporary-mm (e.g., when BPF programs are
installed), since different PGDs are used for the task memory.

Finally, we randomize the address of the PTEs to harden against exploits
that use these PTEs.

Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Masami Hiramatsu <mhiramat@kernel.org>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/include/asm/pgtable.h       |  3 +++
 arch/x86/include/asm/text-patching.h |  2 ++
 arch/x86/kernel/alternative.c        |  3 +++
 arch/x86/mm/init_64.c                | 35 ++++++++++++++++++++++++++++
 init/main.c                          |  3 +++
 5 files changed, 46 insertions(+)

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 40616e805292..e8f630d9a2ed 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -1021,6 +1021,9 @@ static inline void __meminit init_trampoline_default(void)
 	/* Default trampoline pgd value */
 	trampoline_pgd_entry = init_top_pgt[pgd_index(__PAGE_OFFSET)];
 }
+
+void __init poking_init(void);
+
 # ifdef CONFIG_RANDOMIZE_MEMORY
 void __meminit init_trampoline(void);
 # else
diff --git a/arch/x86/include/asm/text-patching.h b/arch/x86/include/asm/text-patching.h
index f8fc8e86cf01..a75eed841eed 100644
--- a/arch/x86/include/asm/text-patching.h
+++ b/arch/x86/include/asm/text-patching.h
@@ -39,5 +39,7 @@ extern void *text_poke_kgdb(void *addr, const void *opcode, size_t len);
 extern int poke_int3_handler(struct pt_regs *regs);
 extern void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler);
 extern int after_bootmem;
+extern __ro_after_init struct mm_struct *poking_mm;
+extern __ro_after_init unsigned long poking_addr;
 
 #endif /* _ASM_X86_TEXT_PATCHING_H */
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index c6a3a10a2fd5..57fdde308bb6 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -678,6 +678,9 @@ void *__init_or_module text_poke_early(void *addr, const void *opcode,
 	return addr;
 }
 
+__ro_after_init struct mm_struct *poking_mm;
+__ro_after_init unsigned long poking_addr;
+
 static void *__text_poke(void *addr, const void *opcode, size_t len)
 {
 	unsigned long flags;
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 5fab264948c2..356a28569a19 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -53,6 +53,7 @@
 #include <asm/init.h>
 #include <asm/uv/uv.h>
 #include <asm/setup.h>
+#include <asm/text-patching.h>
 
 #include "mm_internal.h"
 
@@ -1388,6 +1389,40 @@ unsigned long memory_block_size_bytes(void)
 	return memory_block_size_probed;
 }
 
+/*
+ * Initialize an mm_struct to be used during poking and a pointer to be used
+ * during patching.
+ */
+void __init poking_init(void)
+{
+	spinlock_t *ptl;
+	pte_t *ptep;
+
+	poking_mm = copy_init_mm();
+	BUG_ON(!poking_mm);
+
+	/*
+	 * Randomize the poking address, but make sure that the following page
+	 * will be mapped at the same PMD. We need 2 pages, so find space for 3,
+	 * and adjust the address if the PMD ends after the first one.
+	 */
+	poking_addr = TASK_UNMAPPED_BASE +
+		(kaslr_get_random_long("Poking") & PAGE_MASK) %
+		(TASK_SIZE - TASK_UNMAPPED_BASE - 3 * PAGE_SIZE);
+
+	if (((poking_addr + PAGE_SIZE) & ~PMD_MASK) == 0)
+		poking_addr += PAGE_SIZE;
+
+	/*
+	 * We need to trigger the allocation of the page-tables that will be
+	 * needed for poking now. Later, poking may be performed in an atomic
+	 * section, which might cause allocation to fail.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+	BUG_ON(!ptep);
+	pte_unmap_unlock(ptep, ptl);
+}
+
 #ifdef CONFIG_SPARSEMEM_VMEMMAP
 /*
  * Initialise the sparsemem vmemmap using huge-pages at the PMD level.
diff --git a/init/main.c b/init/main.c
index ee147103ba1b..a461150adfb1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -497,6 +497,8 @@ void __init __weak thread_stack_cache_init(void)
 
 void __init __weak mem_encrypt_init(void) { }
 
+void __init __weak poking_init(void) { }
+
 bool initcall_debug;
 core_param(initcall_debug, initcall_debug, bool, 0644);
 
@@ -731,6 +733,7 @@ asmlinkage __visible void __init start_kernel(void)
 	taskstats_init_early();
 	delayacct_init();
 
+	poking_init();
 	check_bugs();
 
 	acpi_subsystem_init();
-- 
2.17.1

[PATCH v7 06/14] x86/alternative: use temporary mm for text poking

[Nadav Amit]

text_poke() can potentially compromise the security as it sets temporary
PTEs in the fixmap. These PTEs might be used to rewrite the kernel code
from other cores accidentally or maliciously, if an attacker gains the
ability to write onto kernel memory.

Moreover, since remote TLBs are not flushed after the temporary PTEs are
removed, the time-window in which the code is writable is not limited if
the fixmap PTEs - maliciously or accidentally - are cached in the TLB.
To address these potential security hazards, we use a temporary mm for
patching the code.

Finally, text_poke() is also not conservative enough when mapping pages,
as it always tries to map 2 pages, even when a single one is sufficient.
So try to be more conservative, and do not map more than needed.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/include/asm/fixmap.h |   2 -
 arch/x86/kernel/alternative.c | 109 +++++++++++++++++++++++++++-------
 arch/x86/xen/mmu_pv.c         |   2 -
 3 files changed, 87 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h
index 50ba74a34a37..9da8cccdf3fb 100644
--- a/arch/x86/include/asm/fixmap.h
+++ b/arch/x86/include/asm/fixmap.h
@@ -103,8 +103,6 @@ enum fixed_addresses {
 #ifdef CONFIG_PARAVIRT
 	FIX_PARAVIRT_BOOTMAP,
 #endif
-	FIX_TEXT_POKE1,	/* reserve 2 pages for text_poke() */
-	FIX_TEXT_POKE0, /* first page is last, because allocation is backward */
 #ifdef	CONFIG_X86_INTEL_MID
 	FIX_LNW_VRTC,
 #endif
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 57fdde308bb6..8fc4685f3117 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -11,6 +11,7 @@
 #include <linux/stop_machine.h>
 #include <linux/slab.h>
 #include <linux/kdebug.h>
+#include <linux/mmu_context.h>
 #include <asm/text-patching.h>
 #include <asm/alternative.h>
 #include <asm/sections.h>
@@ -683,41 +684,105 @@ __ro_after_init unsigned long poking_addr;
 
 static void *__text_poke(void *addr, const void *opcode, size_t len)
 {
+	bool cross_page_boundary = offset_in_page(addr) + len > PAGE_SIZE;
+	temporary_mm_state_t prev;
+	struct page *pages[2] = {NULL};
 	unsigned long flags;
-	char *vaddr;
-	struct page *pages[2];
-	int i;
+	pte_t pte, *ptep;
+	spinlock_t *ptl;
 
 	/*
-	 * While boot memory allocator is runnig we cannot use struct
-	 * pages as they are not yet initialized.
+	 * While boot memory allocator is running we cannot use struct pages as
+	 * they are not yet initialized.
 	 */
 	BUG_ON(!after_bootmem);
 
 	if (!core_kernel_text((unsigned long)addr)) {
 		pages[0] = vmalloc_to_page(addr);
-		pages[1] = vmalloc_to_page(addr + PAGE_SIZE);
+		if (cross_page_boundary)
+			pages[1] = vmalloc_to_page(addr + PAGE_SIZE);
 	} else {
 		pages[0] = virt_to_page(addr);
 		WARN_ON(!PageReserved(pages[0]));
-		pages[1] = virt_to_page(addr + PAGE_SIZE);
+		if (cross_page_boundary)
+			pages[1] = virt_to_page(addr + PAGE_SIZE);
 	}
-	BUG_ON(!pages[0]);
+	BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
+
 	local_irq_save(flags);
-	set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[0]));
-	if (pages[1])
-		set_fixmap(FIX_TEXT_POKE1, page_to_phys(pages[1]));
-	vaddr = (char *)fix_to_virt(FIX_TEXT_POKE0);
-	memcpy(&vaddr[(unsigned long)addr & ~PAGE_MASK], opcode, len);
-	clear_fixmap(FIX_TEXT_POKE0);
-	if (pages[1])
-		clear_fixmap(FIX_TEXT_POKE1);
-	local_flush_tlb();
-	sync_core();
-	/* Could also do a CLFLUSH here to speed up CPU recovery; but
-	   that causes hangs on some VIA CPUs. */
-	for (i = 0; i < len; i++)
-		BUG_ON(((char *)addr)[i] != ((char *)opcode)[i]);
+
+	/*
+	 * The lock is not really needed, but this allows to avoid open-coding.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+
+	/*
+	 * This must not fail; preallocated in poking_init().
+	 */
+	VM_BUG_ON(!ptep);
+
+	pte = mk_pte(pages[0], PAGE_KERNEL);
+	set_pte_at(poking_mm, poking_addr, ptep, pte);
+
+	if (cross_page_boundary) {
+		pte = mk_pte(pages[1], PAGE_KERNEL);
+		set_pte_at(poking_mm, poking_addr + PAGE_SIZE, ptep + 1, pte);
+	}
+
+	/*
+	 * Loading the temporary mm behaves as a compiler barrier, which
+	 * guarantees that the PTE will be set at the time memcpy() is done.
+	 */
+	prev = use_temporary_mm(poking_mm);
+
+	kasan_disable_current();
+	memcpy((u8 *)poking_addr + offset_in_page(addr), opcode, len);
+	kasan_enable_current();
+
+	/*
+	 * Ensure that the PTE is only cleared after the instructions of memcpy
+	 * were issued by using a compiler barrier.
+	 */
+	barrier();
+
+	pte_clear(poking_mm, poking_addr, ptep);
+
+	/*
+	 * __flush_tlb_one_user() performs a redundant TLB flush when PTI is on,
+	 * as it also flushes the corresponding "user" address spaces, which
+	 * does not exist.
+	 *
+	 * Poking, however, is already very inefficient since it does not try to
+	 * batch updates, so we ignore this problem for the time being.
+	 *
+	 * Since the PTEs do not exist in other kernel address-spaces, we do
+	 * not use __flush_tlb_one_kernel(), which when PTI is on would cause
+	 * more unwarranted TLB flushes.
+	 *
+	 * There is a slight anomaly here: the PTE is a supervisor-only and
+	 * (potentially) global and we use __flush_tlb_one_user() but this
+	 * should be fine.
+	 */
+	__flush_tlb_one_user(poking_addr);
+	if (cross_page_boundary) {
+		pte_clear(poking_mm, poking_addr + PAGE_SIZE, ptep + 1);
+		__flush_tlb_one_user(poking_addr + PAGE_SIZE);
+	}
+
+	/*
+	 * Loading the previous page-table hierarchy requires a serializing
+	 * instruction that already allows the core to see the updated version.
+	 * Xen-PV is assumed to serialize execution in a similar manner.
+	 */
+	unuse_temporary_mm(prev);
+
+	pte_unmap_unlock(ptep, ptl);
+	/*
+	 * If the text doesn't match what we just wrote; something is
+	 * fundamentally screwy, there's nothing we can really do about that.
+	 */
+	BUG_ON(memcmp(addr, opcode, len));
+
 	local_irq_restore(flags);
 	return addr;
 }
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index a5d7ed125337..e9b474c396d2 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -2318,8 +2318,6 @@ static void xen_set_fixmap(unsigned idx, phys_addr_t phys, pgprot_t prot)
 #elif defined(CONFIG_X86_VSYSCALL_EMULATION)
 	case VSYSCALL_PAGE:
 #endif
-	case FIX_TEXT_POKE0:
-	case FIX_TEXT_POKE1:
 		/* All local page mappings */
 		pte = pfn_pte(phys, prot);
 		break;
-- 
2.17.1

[PATCH v7 07/14] x86/kgdb: avoid redundant comparison of patched code

[Nadav Amit]

text_poke() already ensures that the written value is the correct one
and fails if that is not the case. There is no need for an additional
comparison. Remove it.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/kernel/kgdb.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index 2636ca8394bd..86484510bf54 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -751,7 +751,6 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long ip)
 int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 {
 	int err;
-	char opc[BREAK_INSTR_SIZE];
 
 	bpt->type = BP_BREAKPOINT;
 	err = probe_kernel_read(bpt->saved_instr, (char *)bpt->bpt_addr,
@@ -770,11 +769,6 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 		return -EBUSY;
 	text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
 		       BREAK_INSTR_SIZE);
-	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
-	if (err)
-		return err;
-	if (memcmp(opc, arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE))
-		return -EINVAL;
 	bpt->type = BP_POKE_BREAKPOINT;
 
 	return err;
@@ -782,9 +776,6 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 
 int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 {
-	int err;
-	char opc[BREAK_INSTR_SIZE];
-
 	if (bpt->type != BP_POKE_BREAKPOINT)
 		goto knl_write;
 	/*
@@ -795,10 +786,7 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 		goto knl_write;
 	text_poke_kgdb((void *)bpt->bpt_addr, bpt->saved_instr,
 		       BREAK_INSTR_SIZE);
-	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
-	if (err || memcmp(opc, bpt->saved_instr, BREAK_INSTR_SIZE))
-		goto knl_write;
-	return err;
+	return 0;
 
 knl_write:
 	return probe_kernel_write((char *)bpt->bpt_addr,
-- 
2.17.1

[PATCH v7 08/14] x86/ftrace: Use text_poke_*() infrastructure

[Nadav Amit]

A following patch is going to make module allocated memory
non-executable. This requires to modify ftrace and make the memory
executable again after it is configured.

In addition, this patch makes ftrace use the general text poking
infrastructure instead ftrace's homegrown text patching. This provides
the advantages of having slightly "safer" code patching and avoiding
races with module removal or other mechanisms that patch the kernel
code.

Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/kernel/ftrace.c | 74 +++++++++++++---------------------------
 1 file changed, 23 insertions(+), 51 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 01ebcb6f263e..f05a0f9e2837 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -22,6 +22,7 @@
 #include <linux/init.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/memory.h>
 
 #include <trace/syscall.h>
 
@@ -29,23 +30,10 @@
 #include <asm/kprobes.h>
 #include <asm/ftrace.h>
 #include <asm/nops.h>
+#include <asm/text-patching.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
-int ftrace_arch_code_modify_prepare(void)
-{
-	set_kernel_text_rw();
-	set_all_modules_text_rw();
-	return 0;
-}
-
-int ftrace_arch_code_modify_post_process(void)
-{
-	set_all_modules_text_ro();
-	set_kernel_text_ro();
-	return 0;
-}
-
 union ftrace_code_union {
 	char code[MCOUNT_INSN_SIZE];
 	struct {
@@ -79,22 +67,6 @@ within(unsigned long addr, unsigned long start, unsigned long end)
 	return addr >= start && addr < end;
 }
 
-static unsigned long text_ip_addr(unsigned long ip)
-{
-	/*
-	 * On x86_64, kernel text mappings are mapped read-only, so we use
-	 * the kernel identity mapping instead of the kernel text mapping
-	 * to modify the kernel text.
-	 *
-	 * For 32bit kernels, these mappings are same and we can use
-	 * kernel identity mapping to modify code.
-	 */
-	if (within(ip, (unsigned long)_text, (unsigned long)_etext))
-		ip = (unsigned long)__va(__pa_symbol(ip));
-
-	return ip;
-}
-
 static const unsigned char *ftrace_nop_replace(void)
 {
 	return ideal_nops[NOP_ATOMIC5];
@@ -124,13 +96,8 @@ ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code,
 	if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
 		return -EINVAL;
 
-	ip = text_ip_addr(ip);
-
 	/* replace the text with the new text */
-	if (probe_kernel_write((void *)ip, new_code, MCOUNT_INSN_SIZE))
-		return -EPERM;
-
-	sync_core();
+	text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
 
 	return 0;
 }
@@ -302,10 +269,7 @@ int ftrace_int3_handler(struct pt_regs *regs)
 
 static int ftrace_write(unsigned long ip, const char *val, int size)
 {
-	ip = text_ip_addr(ip);
-
-	if (probe_kernel_write((void *)ip, val, size))
-		return -EPERM;
+	text_poke((void *)ip, val, size);
 
 	return 0;
 }
@@ -653,9 +617,11 @@ void arch_ftrace_update_code(int command)
 {
 	/* See comment above by declaration of modifying_ftrace_code */
 	atomic_inc(&modifying_ftrace_code);
+	mutex_lock(&text_mutex);
 
 	ftrace_modify_all_code(command);
 
+	mutex_unlock(&text_mutex);
 	atomic_dec(&modifying_ftrace_code);
 }
 
@@ -741,6 +707,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	unsigned long end_offset;
 	unsigned long op_offset;
 	unsigned long offset;
+	unsigned long npages;
 	unsigned long size;
 	unsigned long ip;
 	unsigned long *ptr;
@@ -748,7 +715,6 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	/* 48 8b 15 <offset> is movq <offset>(%rip), %rdx */
 	unsigned const char op_ref[] = { 0x48, 0x8b, 0x15 };
 	union ftrace_op_code_union op_ptr;
-	int ret;
 
 	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
 		start_offset = (unsigned long)ftrace_regs_caller;
@@ -772,19 +738,16 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 		return 0;
 
 	*tramp_size = size + MCOUNT_INSN_SIZE + sizeof(void *);
+	npages = DIV_ROUND_UP(*tramp_size, PAGE_SIZE);
 
 	/* Copy ftrace_caller onto the trampoline memory */
-	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
-	if (WARN_ON(ret < 0)) {
-		tramp_free(trampoline, *tramp_size);
-		return 0;
-	}
+	text_poke_early(trampoline, (void *)start_offset, size);
 
 	ip = (unsigned long)trampoline + size;
 
 	/* The trampoline ends with a jmp to ftrace_epilogue */
 	jmp = ftrace_jmp_replace(ip, (unsigned long)ftrace_epilogue);
-	memcpy(trampoline + size, jmp, MCOUNT_INSN_SIZE);
+	text_poke_early(trampoline + size, jmp, MCOUNT_INSN_SIZE);
 
 	/*
 	 * The address of the ftrace_ops that is used for this trampoline
@@ -813,11 +776,19 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	op_ptr.offset = offset;
 
 	/* put in the new offset to the ftrace_ops */
-	memcpy(trampoline + op_offset, &op_ptr, OP_REF_SIZE);
+	text_poke_early(trampoline + op_offset, &op_ptr, OP_REF_SIZE);
 
 	/* ALLOC_TRAMP flags lets us know we created it */
 	ops->flags |= FTRACE_OPS_FL_ALLOC_TRAMP;
 
+	set_memory_ro((unsigned long)trampoline, npages);
+
+	/*
+	 * TODO: Once we have better code (and page-table) protection
+	 * mechanisms, ensure that the code has not been tampered before.
+	 */
+	set_memory_x((unsigned long)trampoline, npages);
+
 	return (unsigned long)trampoline;
 }
 
@@ -853,8 +824,6 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
 		 */
 		if (!(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
 			return;
-		npages = PAGE_ALIGN(ops->trampoline_size) >> PAGE_SHIFT;
-		set_memory_rw(ops->trampoline, npages);
 	} else {
 		ops->trampoline = create_trampoline(ops, &size);
 		if (!ops->trampoline)
@@ -863,6 +832,8 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
 		npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
 	}
 
+	mutex_lock(&text_mutex);
+
 	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
 	ip = ops->trampoline + offset;
 
@@ -871,7 +842,8 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
 	/* Do a safe modify in case the trampoline is executing */
 	new = ftrace_call_replace(ip, (unsigned long)func);
 	ret = update_ftrace_func(ip, new);
-	set_memory_ro(ops->trampoline, npages);
+
+	mutex_unlock(&text_mutex);
 
 	/* The update should never fail */
 	WARN_ON(ret);
-- 
2.17.1

Re: [PATCH v7 08/14] x86/ftrace: Use text_poke_*() infrastructure

[Nadav Amit]

> On Dec 4, 2018, at 5:34 PM, Nadav Amit <namit@vmware.com> wrote:
> 
> A following patch is going to make module allocated memory
> non-executable. This requires to modify ftrace and make the memory
> executable again after it is configured.
> 
> In addition, this patch makes ftrace use the general text poking
> infrastructure instead ftrace's homegrown text patching. This provides
> the advantages of having slightly "safer" code patching and avoiding
> races with module removal or other mechanisms that patch the kernel
> code.
> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
> arch/x86/kernel/ftrace.c | 74 +++++++++++++---------------------------
> 1 file changed, 23 insertions(+), 51 deletions(-)

Steven Rostedt pointed that using text_poke() instead of
probe_kernel_write() would introduce considerable overheads. Running:

  # time { echo function > current_tracer; } 

takes 0.24s without this patch and 0.7s with. I don’t know whether to
consider it “so bad”. Obviously we can introduce a batching mechanism and/or
do some micro-optimization (the latter will not buy us much though).

Anyhow, in the meanwhile Steven asked that we’ll leave out the changes in
this patch-set, excluding the set_memory_x() that we need after calling
module_alloc(), and consider them later.

Re: [PATCH v7 08/14] x86/ftrace: Use text_poke_*() infrastructure

[Ingo Molnar]

* Nadav Amit <namit@vmware.com> wrote:

> > On Dec 4, 2018, at 5:34 PM, Nadav Amit <namit@vmware.com> wrote:
> > 
> > A following patch is going to make module allocated memory
> > non-executable. This requires to modify ftrace and make the memory
> > executable again after it is configured.
> > 
> > In addition, this patch makes ftrace use the general text poking
> > infrastructure instead ftrace's homegrown text patching. This provides
> > the advantages of having slightly "safer" code patching and avoiding
> > races with module removal or other mechanisms that patch the kernel
> > code.
> > 
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Signed-off-by: Nadav Amit <namit@vmware.com>
> > ---
> > arch/x86/kernel/ftrace.c | 74 +++++++++++++---------------------------
> > 1 file changed, 23 insertions(+), 51 deletions(-)
> 
> Steven Rostedt pointed that using text_poke() instead of
> probe_kernel_write() would introduce considerable overheads. Running:
> 
>   # time { echo function > current_tracer; } 
> 
> takes 0.24s without this patch and 0.7s with. I don’t know whether to
> consider it “so bad”. Obviously we can introduce a batching mechanism and/or
> do some micro-optimization (the latter will not buy us much though).

This should definitely not regress, so can we try the batching approach?

Thanks,

	Ingo

[PATCH v7 09/14] x86/kprobes: Instruction pages initialization enhancements

[Nadav Amit]

This patch is a preparatory patch for a following patch that makes
module allocated pages non-executable. The patch sets the page as
executable after allocation.

In the future, we may get better protection of executables. For example,
by using hypercalls to request the hypervisor to protect VM executable
pages from modifications using nested page-tables. This would allow
us to ensure the executable has not changed between allocation and
its write-protection.

While at it, do some small cleanup of what appears to be unnecessary
masking.

Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/kernel/kprobes/core.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index c33b06f5faa4..ca0118d3b3e8 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -431,8 +431,20 @@ void *alloc_insn_page(void)
 	void *page;
 
 	page = module_alloc(PAGE_SIZE);
-	if (page)
-		set_memory_ro((unsigned long)page & PAGE_MASK, 1);
+	if (page == NULL)
+		return NULL;
+
+	/*
+	 * First make the page read-only, and then only then make it executable
+	 * to prevent it from being W+X in between.
+	 */
+	set_memory_ro((unsigned long)page, 1);
+
+	/*
+	 * TODO: Once additional kernel code protection mechanisms are set, ensure
+	 * that the page was not maliciously altered and it is still zeroed.
+	 */
+	set_memory_x((unsigned long)page, 1);
 
 	return page;
 }
@@ -440,8 +452,12 @@ void *alloc_insn_page(void)
 /* Recover page to RW mode before releasing it */
 void free_insn_page(void *page)
 {
-	set_memory_nx((unsigned long)page & PAGE_MASK, 1);
-	set_memory_rw((unsigned long)page & PAGE_MASK, 1);
+	/*
+	 * First make the page non-executable, and then only then make it
+	 * writable to prevent it from being W+X in between.
+	 */
+	set_memory_nx((unsigned long)page, 1);
+	set_memory_rw((unsigned long)page, 1);
 	module_memfree(page);
 }
 
-- 
2.17.1

Re: [PATCH v7 09/14] x86/kprobes: Instruction pages initialization enhancements

[Masami Hiramatsu]

On Tue, 4 Dec 2018 17:34:03 -0800
Nadav Amit <namit@vmware.com> wrote:

> This patch is a preparatory patch for a following patch that makes
> module allocated pages non-executable. The patch sets the page as
> executable after allocation.
> 
> In the future, we may get better protection of executables. For example,
> by using hypercalls to request the hypervisor to protect VM executable
> pages from modifications using nested page-tables. This would allow
> us to ensure the executable has not changed between allocation and
> its write-protection.

Sounds interesting!

> 
> While at it, do some small cleanup of what appears to be unnecessary
> masking.

Looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>


Thanks!

> 
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  arch/x86/kernel/kprobes/core.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index c33b06f5faa4..ca0118d3b3e8 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -431,8 +431,20 @@ void *alloc_insn_page(void)
>  	void *page;
>  
>  	page = module_alloc(PAGE_SIZE);
> -	if (page)
> -		set_memory_ro((unsigned long)page & PAGE_MASK, 1);
> +	if (page == NULL)
> +		return NULL;
> +
> +	/*
> +	 * First make the page read-only, and then only then make it executable
> +	 * to prevent it from being W+X in between.
> +	 */
> +	set_memory_ro((unsigned long)page, 1);
> +
> +	/*
> +	 * TODO: Once additional kernel code protection mechanisms are set, ensure
> +	 * that the page was not maliciously altered and it is still zeroed.
> +	 */
> +	set_memory_x((unsigned long)page, 1);
>  
>  	return page;
>  }
> @@ -440,8 +452,12 @@ void *alloc_insn_page(void)
>  /* Recover page to RW mode before releasing it */
>  void free_insn_page(void *page)
>  {
> -	set_memory_nx((unsigned long)page & PAGE_MASK, 1);
> -	set_memory_rw((unsigned long)page & PAGE_MASK, 1);
> +	/*
> +	 * First make the page non-executable, and then only then make it
> +	 * writable to prevent it from being W+X in between.
> +	 */
> +	set_memory_nx((unsigned long)page, 1);
> +	set_memory_rw((unsigned long)page, 1);
>  	module_memfree(page);
>  }
>  
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH v7 10/14] x86: avoid W^X being broken during modules loading

[Nadav Amit]

When modules and BPF filters are loaded, there is a time window in
which some memory is both writable and executable. An attacker that has
already found another vulnerability (e.g., a dangling pointer) might be
able to exploit this behavior to overwrite kernel code. This patch
prevents having writable executable PTEs in this stage.

In addition, avoiding having R+X mappings can also slightly simplify the
patching of modules code on initialization (e.g., by alternatives and
static-key), as would be done in the next patch. This was actually the
main motivation for this patch.

To avoid having W+X mappings, set them initially as RW (NX) and after
they are set as RO set them as X as well. Setting them as executable is
done as a separate step to avoid one core in which the old PTE is cached
(hence writable), and another which sees the updated PTE (executable),
which would break the W^X protection.

Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/kernel/alternative.c | 28 +++++++++++++++++++++-------
 arch/x86/kernel/module.c      |  2 +-
 include/linux/filter.h        |  6 ++++++
 kernel/module.c               | 10 ++++++++++
 4 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 8fc4685f3117..18415e3b6000 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -667,15 +667,29 @@ void __init alternative_instructions(void)
  * handlers seeing an inconsistent instruction while you patch.
  */
 void *__init_or_module text_poke_early(void *addr, const void *opcode,
-					      size_t len)
+				       size_t len)
 {
 	unsigned long flags;
-	local_irq_save(flags);
-	memcpy(addr, opcode, len);
-	local_irq_restore(flags);
-	sync_core();
-	/* Could also do a CLFLUSH here to speed up CPU recovery; but
-	   that causes hangs on some VIA CPUs. */
+
+	if (static_cpu_has(X86_FEATURE_NX) &&
+	    is_module_text_address((unsigned long)addr)) {
+		/*
+		 * Modules text is marked initially as non-executable, so the
+		 * code cannot be running and speculative code-fetches are
+		 * prevented. We can just change the code.
+		 */
+		memcpy(addr, opcode, len);
+	} else {
+		local_irq_save(flags);
+		memcpy(addr, opcode, len);
+		local_irq_restore(flags);
+		sync_core();
+
+		/*
+		 * Could also do a CLFLUSH here to speed up CPU recovery; but
+		 * that causes hangs on some VIA CPUs.
+		 */
+	}
 	return addr;
 }
 
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index b052e883dd8c..cfa3106faee4 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -87,7 +87,7 @@ void *module_alloc(unsigned long size)
 	p = __vmalloc_node_range(size, MODULE_ALIGN,
 				    MODULES_VADDR + get_module_load_offset(),
 				    MODULES_END, GFP_KERNEL,
-				    PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
+				    PAGE_KERNEL, 0, NUMA_NO_NODE,
 				    __builtin_return_address(0));
 	if (p && (kasan_module_alloc(p, size) < 0)) {
 		vfree(p);
diff --git a/include/linux/filter.h b/include/linux/filter.h
index de629b706d1d..ee9ae03c5f56 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -704,7 +704,13 @@ static inline void bpf_prog_unlock_ro(struct bpf_prog *fp)
 
 static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
 {
+	/*
+	 * Perform mapping changes in two stages to avoid opening a time-window
+	 * in which a PTE is cached in any TLB as writable, but marked as
+	 * executable in the memory-resident mappings (e.g., page-tables).
+	 */
 	set_memory_ro((unsigned long)hdr, hdr->pages);
+	set_memory_x((unsigned long)hdr, hdr->pages);
 }
 
 static inline void bpf_jit_binary_unlock_ro(struct bpf_binary_header *hdr)
diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..7cb207249437 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1946,9 +1946,19 @@ void module_enable_ro(const struct module *mod, bool after_init)
 	if (!rodata_enabled)
 		return;
 
+	/*
+	 * Perform mapping changes in two stages to avoid opening a time-window
+	 * in which a PTE is cached in any TLB as writable, but marked as
+	 * executable in the memory-resident mappings (e.g., page-tables).
+	 */
 	frob_text(&mod->core_layout, set_memory_ro);
+	frob_text(&mod->core_layout, set_memory_x);
+
 	frob_rodata(&mod->core_layout, set_memory_ro);
+
 	frob_text(&mod->init_layout, set_memory_ro);
+	frob_text(&mod->init_layout, set_memory_x);
+
 	frob_rodata(&mod->init_layout, set_memory_ro);
 
 	if (after_init)
-- 
2.17.1

[PATCH v7 11/14] x86/jump-label: remove support for custom poker

[Nadav Amit]

There are only two types of poking: early and breakpoint based. The use
of a function pointer to perform poking complicates the code and is
probably inefficient due to the use of indirect branches.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/kernel/jump_label.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index ed5fe274a7d8..994c13e2867d 100644
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -39,7 +39,6 @@ static void bug_at(unsigned char *ip, int line)
 
 static void __ref __jump_label_transform(struct jump_entry *entry,
 					 enum jump_label_type type,
-					 void *(*poker)(void *, const void *, size_t),
 					 int init)
 {
 	union jump_code_union jmp;
@@ -52,14 +51,6 @@ static void __ref __jump_label_transform(struct jump_entry *entry,
 	jmp.offset = jump_entry_target(entry) -
 		     (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
 
-	/*
-	 * As long as we're UP and not yet marked RO, we can use
-	 * text_poke_early; SYSTEM_BOOTING guarantees both, as we switch to
-	 * SYSTEM_SCHEDULING before going either.
-	 */
-	if (system_state == SYSTEM_BOOTING)
-		poker = text_poke_early;
-
 	if (type == JUMP_LABEL_JMP) {
 		if (init) {
 			expect = default_nop; line = __LINE__;
@@ -82,16 +73,17 @@ static void __ref __jump_label_transform(struct jump_entry *entry,
 		bug_at((void *)jump_entry_code(entry), line);
 
 	/*
-	 * Make text_poke_bp() a default fallback poker.
+	 * As long as we're UP and not yet marked RO, we can use
+	 * text_poke_early; SYSTEM_BOOTING guarantees both, as we switch to
+	 * SYSTEM_SCHEDULING before going either.
 	 *
 	 * At the time the change is being done, just ignore whether we
 	 * are doing nop -> jump or jump -> nop transition, and assume
 	 * always nop being the 'currently valid' instruction
-	 *
 	 */
-	if (poker) {
-		(*poker)((void *)jump_entry_code(entry), code,
-			 JUMP_LABEL_NOP_SIZE);
+	if (init || system_state == SYSTEM_BOOTING) {
+		text_poke_early((void *)jump_entry_code(entry), code,
+				JUMP_LABEL_NOP_SIZE);
 		return;
 	}
 
@@ -103,7 +95,7 @@ void arch_jump_label_transform(struct jump_entry *entry,
 			       enum jump_label_type type)
 {
 	mutex_lock(&text_mutex);
-	__jump_label_transform(entry, type, NULL, 0);
+	__jump_label_transform(entry, type, 0);
 	mutex_unlock(&text_mutex);
 }
 
@@ -133,7 +125,7 @@ __init_or_module void arch_jump_label_transform_static(struct jump_entry *entry,
 			jlstate = JL_STATE_NO_UPDATE;
 	}
 	if (jlstate == JL_STATE_UPDATE)
-		__jump_label_transform(entry, type, text_poke_early, 1);
+		__jump_label_transform(entry, type, 1);
 }
 
 #endif
-- 
2.17.1

[PATCH v7 12/14] x86/alternative: Remove the return value of text_poke_*()

[Nadav Amit]

The return value of text_poke_early() and text_poke_bp() is useless.
Remove it.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/include/asm/text-patching.h |  4 ++--
 arch/x86/kernel/alternative.c        | 11 ++++-------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/text-patching.h b/arch/x86/include/asm/text-patching.h
index a75eed841eed..c90678fd391a 100644
--- a/arch/x86/include/asm/text-patching.h
+++ b/arch/x86/include/asm/text-patching.h
@@ -18,7 +18,7 @@ static inline void apply_paravirt(struct paravirt_patch_site *start,
 #define __parainstructions_end	NULL
 #endif
 
-extern void *text_poke_early(void *addr, const void *opcode, size_t len);
+extern void text_poke_early(void *addr, const void *opcode, size_t len);
 
 /*
  * Clear and restore the kernel write-protection flag on the local CPU.
@@ -37,7 +37,7 @@ extern void *text_poke_early(void *addr, const void *opcode, size_t len);
 extern void *text_poke(void *addr, const void *opcode, size_t len);
 extern void *text_poke_kgdb(void *addr, const void *opcode, size_t len);
 extern int poke_int3_handler(struct pt_regs *regs);
-extern void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler);
+extern void text_poke_bp(void *addr, const void *opcode, size_t len, void *handler);
 extern int after_bootmem;
 extern __ro_after_init struct mm_struct *poking_mm;
 extern __ro_after_init unsigned long poking_addr;
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 18415e3b6000..2740ad2c6f21 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -264,7 +264,7 @@ static void __init_or_module add_nops(void *insns, unsigned int len)
 
 extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
 extern s32 __smp_locks[], __smp_locks_end[];
-void *text_poke_early(void *addr, const void *opcode, size_t len);
+void text_poke_early(void *addr, const void *opcode, size_t len);
 
 /*
  * Are we looking at a near JMP with a 1 or 4-byte displacement.
@@ -666,8 +666,8 @@ void __init alternative_instructions(void)
  * instructions. And on the local CPU you need to be protected again NMI or MCE
  * handlers seeing an inconsistent instruction while you patch.
  */
-void *__init_or_module text_poke_early(void *addr, const void *opcode,
-				       size_t len)
+void __init_or_module text_poke_early(void *addr, const void *opcode,
+				      size_t len)
 {
 	unsigned long flags;
 
@@ -690,7 +690,6 @@ void *__init_or_module text_poke_early(void *addr, const void *opcode,
 		 * that causes hangs on some VIA CPUs.
 		 */
 	}
-	return addr;
 }
 
 __ro_after_init struct mm_struct *poking_mm;
@@ -893,7 +892,7 @@ int poke_int3_handler(struct pt_regs *regs)
  *	  replacing opcode
  *	- sync cores
  */
-void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler)
+void text_poke_bp(void *addr, const void *opcode, size_t len, void *handler)
 {
 	unsigned char int3 = 0xcc;
 
@@ -935,7 +934,5 @@ void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler)
 	 * the writing of the new instruction.
 	 */
 	bp_patching_in_progress = false;
-
-	return addr;
 }
 
-- 
2.17.1

[PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Nadav Amit]

When module memory is about to be freed, there is no apparent reason to
make it (and its data) executable, but that's exactly what is done
today. This is not efficient and not secure.

There are various theories why it was done, but none of them seem as
something that really require it today. nios2 uses kmalloc for module
memory, but anyhow it does not change the PTEs of the module memory.  In
x86, changing vmalloc'd memory mappings also modifies the direct mapping
alias, but the NX-bit is not modified in such way.

So let's remove it. Andy suggested that the changes of the PTEs can be
avoided (excluding the direct-mapping alias), which is true. However,
in x86 it requires some cleanup of the contiguous page allocator, which
is outside of the scope of this patch-set.

Cc: Rick P Edgecombe <rick.p.edgecombe@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 kernel/module.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 7cb207249437..57c5b23746e7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2027,20 +2027,29 @@ void set_all_modules_text_ro(void)
 	mutex_unlock(&module_mutex);
 }
 
-static void disable_ro_nx(const struct module_layout *layout)
+static void module_restore_mappings(const struct module_layout *layout)
 {
-	if (rodata_enabled) {
-		frob_text(layout, set_memory_rw);
-		frob_rodata(layout, set_memory_rw);
-		frob_ro_after_init(layout, set_memory_rw);
-	}
-	frob_rodata(layout, set_memory_x);
-	frob_ro_after_init(layout, set_memory_x);
-	frob_writable_data(layout, set_memory_x);
+	/*
+	 * First, make the mappings of the code non-executable to prevent
+	 * transient W+X mappings from being set when the text is set as RW.
+	 */
+	frob_text(layout, set_memory_nx);
+
+	if (!rodata_enabled)
+		return;
+
+	/*
+	 * Second, set the memory as writable. Although the module memory is
+	 * about to be freed, these calls are required (at least on x86) to
+	 * restore the direct map to its "correct" state.
+	 */
+	frob_text(layout, set_memory_rw);
+	frob_rodata(layout, set_memory_rw);
+	frob_ro_after_init(layout, set_memory_rw);
 }
 
 #else
-static void disable_ro_nx(const struct module_layout *layout) { }
+static void module_restore_mappings(const struct module_layout *layout) { }
 static void module_enable_nx(const struct module *mod) { }
 static void module_disable_nx(const struct module *mod) { }
 #endif
@@ -2173,7 +2182,7 @@ static void free_module(struct module *mod)
 	mutex_unlock(&module_mutex);
 
 	/* This may be empty, but that's OK */
-	disable_ro_nx(&mod->init_layout);
+	module_restore_mappings(&mod->init_layout);
 	module_arch_freeing_init(mod);
 	module_memfree(mod->init_layout.base);
 	kfree(mod->args);
@@ -2183,7 +2192,7 @@ static void free_module(struct module *mod)
 	lockdep_free_key_range(mod->core_layout.base, mod->core_layout.size);
 
 	/* Finally, free the core (containing the module structure) */
-	disable_ro_nx(&mod->core_layout);
+	module_restore_mappings(&mod->core_layout);
 	module_memfree(mod->core_layout.base);
 }
 
@@ -3507,7 +3516,7 @@ static noinline int do_init_module(struct module *mod)
 #endif
 	module_enable_ro(mod, true);
 	mod_tree_remove_init(mod);
-	disable_ro_nx(&mod->init_layout);
+	module_restore_mappings(&mod->init_layout);
 	module_arch_freeing_init(mod);
 	mod->init_layout.base = NULL;
 	mod->init_layout.size = 0;
-- 
2.17.1

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Peter Zijlstra]

On Tue, Dec 04, 2018 at 05:34:07PM -0800, Nadav Amit wrote:

> So let's remove it. Andy suggested that the changes of the PTEs can be
> avoided (excluding the direct-mapping alias), which is true. However,
> in x86 it requires some cleanup of the contiguous page allocator, which
> is outside of the scope of this patch-set.

I think x86-cpa stands for change_page_attr() :-)

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Nadav Amit]

> On Dec 6, 2018, at 1:57 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Tue, Dec 04, 2018 at 05:34:07PM -0800, Nadav Amit wrote:
> 
>> So let's remove it. Andy suggested that the changes of the PTEs can be
>> avoided (excluding the direct-mapping alias), which is true. However,
>> in x86 it requires some cleanup of the contiguous page allocator, which
>> is outside of the scope of this patch-set.
> 
> I think x86-cpa stands for change_page_attr() :-)

Thanks - it makes much more sense… I took the first thing that Google
showed.

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Andrea Parri]

On Tue, Dec 04, 2018 at 05:34:07PM -0800, Nadav Amit wrote:
> When module memory is about to be freed, there is no apparent reason to
> make it (and its data) executable, but that's exactly what is done
> today. This is not efficient and not secure.

Looks to me like you forgot to Cc the maintainer of this file: doing it
now.  The same consideration would hold for 14/14.

  Andrea


> 
> There are various theories why it was done, but none of them seem as
> something that really require it today. nios2 uses kmalloc for module
> memory, but anyhow it does not change the PTEs of the module memory.  In
> x86, changing vmalloc'd memory mappings also modifies the direct mapping
> alias, but the NX-bit is not modified in such way.
> 
> So let's remove it. Andy suggested that the changes of the PTEs can be
> avoided (excluding the direct-mapping alias), which is true. However,
> in x86 it requires some cleanup of the contiguous page allocator, which
> is outside of the scope of this patch-set.
> 
> Cc: Rick P Edgecombe <rick.p.edgecombe@intel.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  kernel/module.c | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 7cb207249437..57c5b23746e7 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2027,20 +2027,29 @@ void set_all_modules_text_ro(void)
>  	mutex_unlock(&module_mutex);
>  }
>  
> -static void disable_ro_nx(const struct module_layout *layout)
> +static void module_restore_mappings(const struct module_layout *layout)
>  {
> -	if (rodata_enabled) {
> -		frob_text(layout, set_memory_rw);
> -		frob_rodata(layout, set_memory_rw);
> -		frob_ro_after_init(layout, set_memory_rw);
> -	}
> -	frob_rodata(layout, set_memory_x);
> -	frob_ro_after_init(layout, set_memory_x);
> -	frob_writable_data(layout, set_memory_x);
> +	/*
> +	 * First, make the mappings of the code non-executable to prevent
> +	 * transient W+X mappings from being set when the text is set as RW.
> +	 */
> +	frob_text(layout, set_memory_nx);
> +
> +	if (!rodata_enabled)
> +		return;
> +
> +	/*
> +	 * Second, set the memory as writable. Although the module memory is
> +	 * about to be freed, these calls are required (at least on x86) to
> +	 * restore the direct map to its "correct" state.
> +	 */
> +	frob_text(layout, set_memory_rw);
> +	frob_rodata(layout, set_memory_rw);
> +	frob_ro_after_init(layout, set_memory_rw);
>  }
>  
>  #else
> -static void disable_ro_nx(const struct module_layout *layout) { }
> +static void module_restore_mappings(const struct module_layout *layout) { }
>  static void module_enable_nx(const struct module *mod) { }
>  static void module_disable_nx(const struct module *mod) { }
>  #endif
> @@ -2173,7 +2182,7 @@ static void free_module(struct module *mod)
>  	mutex_unlock(&module_mutex);
>  
>  	/* This may be empty, but that's OK */
> -	disable_ro_nx(&mod->init_layout);
> +	module_restore_mappings(&mod->init_layout);
>  	module_arch_freeing_init(mod);
>  	module_memfree(mod->init_layout.base);
>  	kfree(mod->args);
> @@ -2183,7 +2192,7 @@ static void free_module(struct module *mod)
>  	lockdep_free_key_range(mod->core_layout.base, mod->core_layout.size);
>  
>  	/* Finally, free the core (containing the module structure) */
> -	disable_ro_nx(&mod->core_layout);
> +	module_restore_mappings(&mod->core_layout);
>  	module_memfree(mod->core_layout.base);
>  }
>  
> @@ -3507,7 +3516,7 @@ static noinline int do_init_module(struct module *mod)
>  #endif
>  	module_enable_ro(mod, true);
>  	mod_tree_remove_init(mod);
> -	disable_ro_nx(&mod->init_layout);
> +	module_restore_mappings(&mod->init_layout);
>  	module_arch_freeing_init(mod);
>  	mod->init_layout.base = NULL;
>  	mod->init_layout.size = 0;
> -- 
> 2.17.1
> 

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Andy Lutomirski]

On Wed, Dec 5, 2018 at 12:52 AM Nadav Amit <namit@vmware.com> wrote:
>
> When module memory is about to be freed, there is no apparent reason to
> make it (and its data) executable, but that's exactly what is done
> today. This is not efficient and not secure.
>
> There are various theories why it was done, but none of them seem as
> something that really require it today. nios2 uses kmalloc for module
> memory, but anyhow it does not change the PTEs of the module memory.  In
> x86, changing vmalloc'd memory mappings also modifies the direct mapping
> alias, but the NX-bit is not modified in such way.
>
> So let's remove it. Andy suggested that the changes of the PTEs can be
> avoided (excluding the direct-mapping alias), which is true. However,
> in x86 it requires some cleanup of the contiguous page allocator, which
> is outside of the scope of this patch-set.
>


I'm okay with this, but I'd like to see Rick's stuff get rebased on
top of it and clean it up for real.

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Nadav Amit]

> On Dec 6, 2018, at 10:52 AM, Andy Lutomirski <luto@kernel.org> wrote:
> 
> On Wed, Dec 5, 2018 at 12:52 AM Nadav Amit <namit@vmware.com> wrote:
>> When module memory is about to be freed, there is no apparent reason to
>> make it (and its data) executable, but that's exactly what is done
>> today. This is not efficient and not secure.
>> 
>> There are various theories why it was done, but none of them seem as
>> something that really require it today. nios2 uses kmalloc for module
>> memory, but anyhow it does not change the PTEs of the module memory.  In
>> x86, changing vmalloc'd memory mappings also modifies the direct mapping
>> alias, but the NX-bit is not modified in such way.
>> 
>> So let's remove it. Andy suggested that the changes of the PTEs can be
>> avoided (excluding the direct-mapping alias), which is true. However,
>> in x86 it requires some cleanup of the contiguous page allocator, which
>> is outside of the scope of this patch-set.
> 
> 
> I'm okay with this, but I'd like to see Rick's stuff get rebased on
> top of it and clean it up for real.

Sorry for my laziness. It just seems that every small thing I touch in
regard to W^X or text_poke() is broken, and I need to finish some other
“chores” first.

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Edgecombe, Rick P]

On Thu, 2018-12-06 at 10:52 -0800, Andy Lutomirski wrote:
> On Wed, Dec 5, 2018 at 12:52 AM Nadav Amit <namit@vmware.com> wrote:
> > 
> > When module memory is about to be freed, there is no apparent reason to
> > make it (and its data) executable, but that's exactly what is done
> > today. This is not efficient and not secure.
> > 
> > There are various theories why it was done, but none of them seem as
> > something that really require it today. nios2 uses kmalloc for module
> > memory, but anyhow it does not change the PTEs of the module memory.  In
> > x86, changing vmalloc'd memory mappings also modifies the direct mapping
> > alias, but the NX-bit is not modified in such way.
> > 
> > So let's remove it. Andy suggested that the changes of the PTEs can be
> > avoided (excluding the direct-mapping alias), which is true. However,
> > in x86 it requires some cleanup of the contiguous page allocator, which
> > is outside of the scope of this patch-set.
> > 
> 
> 
> I'm okay with this, but I'd like to see Rick's stuff get rebased on
> top of it and clean it up for real.

Nadav,

Hmm, since you are trying to move things forward and not close all cases in one
swoop, would it make sense to split the modules W^X mission from this patchset?

Thanks,

Rick

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Nadav Amit]

> On Dec 6, 2018, at 12:21 PM, Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote:
> 
> On Thu, 2018-12-06 at 10:52 -0800, Andy Lutomirski wrote:
>> On Wed, Dec 5, 2018 at 12:52 AM Nadav Amit <namit@vmware.com> wrote:
>>> When module memory is about to be freed, there is no apparent reason to
>>> make it (and its data) executable, but that's exactly what is done
>>> today. This is not efficient and not secure.
>>> 
>>> There are various theories why it was done, but none of them seem as
>>> something that really require it today. nios2 uses kmalloc for module
>>> memory, but anyhow it does not change the PTEs of the module memory.  In
>>> x86, changing vmalloc'd memory mappings also modifies the direct mapping
>>> alias, but the NX-bit is not modified in such way.
>>> 
>>> So let's remove it. Andy suggested that the changes of the PTEs can be
>>> avoided (excluding the direct-mapping alias), which is true. However,
>>> in x86 it requires some cleanup of the contiguous page allocator, which
>>> is outside of the scope of this patch-set.
>> 
>> 
>> I'm okay with this, but I'd like to see Rick's stuff get rebased on
>> top of it and clean it up for real.
> 
> Nadav,
> 
> Hmm, since you are trying to move things forward and not close all cases in one
> swoop, would it make sense to split the modules W^X mission from this patchset?

That’s what I tried to “hint”. Tglx asked for the module stuff in one of the
previous versions.

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Jessica Yu]

+++ Nadav Amit [04/12/18 17:34 -0800]:
>When module memory is about to be freed, there is no apparent reason to
>make it (and its data) executable, but that's exactly what is done
>today. This is not efficient and not secure.
>
>There are various theories why it was done, but none of them seem as
>something that really require it today. nios2 uses kmalloc for module
>memory, but anyhow it does not change the PTEs of the module memory.  In
>x86, changing vmalloc'd memory mappings also modifies the direct mapping
>alias, but the NX-bit is not modified in such way.
>
>So let's remove it. Andy suggested that the changes of the PTEs can be
>avoided (excluding the direct-mapping alias), which is true. However,
>in x86 it requires some cleanup of the contiguous page allocator, which
>is outside of the scope of this patch-set.
>
>Cc: Rick P Edgecombe <rick.p.edgecombe@intel.com>
>Cc: Will Deacon <will.deacon@arm.com>
>Cc: Andy Lutomirski <luto@kernel.org>
>Signed-off-by: Nadav Amit <namit@vmware.com>

[ Thanks Andrea Parri for the cc ]

Regarding the patch subject, don't you mean "Do not make module
memory executable" or "Do not unset nx" instead of "Do not set nx"?
Hm, these double negatives are confusing :-)

I think this also needs to be done in the load_module() error path.
See the bug_cleanup label. There, module_disable_{ro,nx}() are called
before module deallocation.

I am not sure why all this was made executable before freeing in the
first place.  Tried to dig through the commit history and the first
commit that introduced this behavior was 448694a1d50 ("module: undo
module RONX protection correctly"). There, the behavior was changed
from W+NX to W+X before releasing the module. But AFAIK from the
changelog, there was no real technical reason behind it, it stemmed
out of the complaint of code asymmetry :-/

Jessica

>---
> kernel/module.c | 35 ++++++++++++++++++++++-------------
> 1 file changed, 22 insertions(+), 13 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 7cb207249437..57c5b23746e7 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2027,20 +2027,29 @@ void set_all_modules_text_ro(void)
> 	mutex_unlock(&module_mutex);
> }
>
>-static void disable_ro_nx(const struct module_layout *layout)
>+static void module_restore_mappings(const struct module_layout *layout)
> {
>-	if (rodata_enabled) {
>-		frob_text(layout, set_memory_rw);
>-		frob_rodata(layout, set_memory_rw);
>-		frob_ro_after_init(layout, set_memory_rw);
>-	}
>-	frob_rodata(layout, set_memory_x);
>-	frob_ro_after_init(layout, set_memory_x);
>-	frob_writable_data(layout, set_memory_x);
>+	/*
>+	 * First, make the mappings of the code non-executable to prevent
>+	 * transient W+X mappings from being set when the text is set as RW.
>+	 */
>+	frob_text(layout, set_memory_nx);
>+
>+	if (!rodata_enabled)
>+		return;
>+
>+	/*
>+	 * Second, set the memory as writable. Although the module memory is
>+	 * about to be freed, these calls are required (at least on x86) to
>+	 * restore the direct map to its "correct" state.
>+	 */
>+	frob_text(layout, set_memory_rw);
>+	frob_rodata(layout, set_memory_rw);
>+	frob_ro_after_init(layout, set_memory_rw);
> }
>
> #else
>-static void disable_ro_nx(const struct module_layout *layout) { }
>+static void module_restore_mappings(const struct module_layout *layout) { }
> static void module_enable_nx(const struct module *mod) { }
> static void module_disable_nx(const struct module *mod) { }
> #endif
>@@ -2173,7 +2182,7 @@ static void free_module(struct module *mod)
> 	mutex_unlock(&module_mutex);
>
> 	/* This may be empty, but that's OK */
>-	disable_ro_nx(&mod->init_layout);
>+	module_restore_mappings(&mod->init_layout);
> 	module_arch_freeing_init(mod);
> 	module_memfree(mod->init_layout.base);
> 	kfree(mod->args);
>@@ -2183,7 +2192,7 @@ static void free_module(struct module *mod)
> 	lockdep_free_key_range(mod->core_layout.base, mod->core_layout.size);
>
> 	/* Finally, free the core (containing the module structure) */
>-	disable_ro_nx(&mod->core_layout);
>+	module_restore_mappings(&mod->core_layout);
> 	module_memfree(mod->core_layout.base);
> }
>
>@@ -3507,7 +3516,7 @@ static noinline int do_init_module(struct module *mod)
> #endif
> 	module_enable_ro(mod, true);
> 	mod_tree_remove_init(mod);
>-	disable_ro_nx(&mod->init_layout);
>+	module_restore_mappings(&mod->init_layout);
> 	module_arch_freeing_init(mod);
> 	mod->init_layout.base = NULL;
> 	mod->init_layout.size = 0;
>-- 
>2.17.1
>

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Nadav Amit]

> On Dec 13, 2018, at 6:10 AM, Jessica Yu <jeyu@kernel.org> wrote:
> 
> +++ Nadav Amit [04/12/18 17:34 -0800]:
>> When module memory is about to be freed, there is no apparent reason to
>> make it (and its data) executable, but that's exactly what is done
>> today. This is not efficient and not secure.
>> 
>> There are various theories why it was done, but none of them seem as
>> something that really require it today. nios2 uses kmalloc for module
>> memory, but anyhow it does not change the PTEs of the module memory.  In
>> x86, changing vmalloc'd memory mappings also modifies the direct mapping
>> alias, but the NX-bit is not modified in such way.
>> 
>> So let's remove it. Andy suggested that the changes of the PTEs can be
>> avoided (excluding the direct-mapping alias), which is true. However,
>> in x86 it requires some cleanup of the contiguous page allocator, which
>> is outside of the scope of this patch-set.
>> 
>> Cc: Rick P Edgecombe <rick.p.edgecombe@intel.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Signed-off-by: Nadav Amit <namit@vmware.com>
> 
> [ Thanks Andrea Parri for the cc ]
> 
> Regarding the patch subject, don't you mean "Do not make module
> memory executable" or "Do not unset nx" instead of "Do not set nx"?
> Hm, these double negatives are confusing :-)

I guess it is just plain wrong in this case… ;-)

> 
> I think this also needs to be done in the load_module() error path.
> See the bug_cleanup label. There, module_disable_{ro,nx}() are called
> before module deallocation.

Yes, I missed this one. I think Rick Edgecombe has a better version of this
patch that also takes care of this case (see
https://lkml.org/lkml/2018/12/11/1573 ). I think he will merge the rest of
this series (although I’m still waiting for Thomas/Ingo to tell me what’s it
going to be with the first patches).

> I am not sure why all this was made executable before freeing in the
> first place.  Tried to dig through the commit history and the first
> commit that introduced this behavior was 448694a1d50 ("module: undo
> module RONX protection correctly"). There, the behavior was changed
> from W+NX to W+X before releasing the module. But AFAIK from the
> changelog, there was no real technical reason behind it, it stemmed
> out of the complaint of code asymmetry :-/

Thanks for looking into it. I gave up after I saw it should have no
architectural reason (on x86) and could not think about such one (on any
arch., certainly for the data). Anyhow, that’s what automatic testing are
for. If this is wrong, things should crash and burn very fast.

[PATCH v7 14/14] module: Prevent module removal racing with text_poke()

[Nadav Amit]

It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 kernel/module.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/module.c b/kernel/module.c
index 57c5b23746e7..b45754961143 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -64,6 +64,7 @@
 #include <linux/bsearch.h>
 #include <linux/dynamic_debug.h>
 #include <linux/audit.h>
+#include <linux/memory.h>
 #include <uapi/linux/module.h>
 #include "module-internal.h"
 
@@ -2181,6 +2182,9 @@ static void free_module(struct module *mod)
 	synchronize_sched();
 	mutex_unlock(&module_mutex);
 
+	/* Protect against patching of the module while it is being removed */
+	mutex_lock(&text_mutex);
+
 	/* This may be empty, but that's OK */
 	module_restore_mappings(&mod->init_layout);
 	module_arch_freeing_init(mod);
@@ -2194,6 +2198,7 @@ static void free_module(struct module *mod)
 	/* Finally, free the core (containing the module structure) */
 	module_restore_mappings(&mod->core_layout);
 	module_memfree(mod->core_layout.base);
+	mutex_unlock(&text_mutex);
 }
 
 void *__symbol_get(const char *symbol)
-- 
2.17.1

Re: [PATCH v7 14/14] module: Prevent module removal racing with text_poke()

[Peter Zijlstra]

On Tue, Dec 04, 2018 at 05:34:08PM -0800, Nadav Amit wrote:
> It seems dangerous to allow code modifications to take place
> concurrently with module unloading. So take the text_mutex while the
> memory of the module is freed.

Fun detail, only x86 seems to actually take text_mutex while poking
text.

> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  kernel/module.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 57c5b23746e7..b45754961143 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -64,6 +64,7 @@
>  #include <linux/bsearch.h>
>  #include <linux/dynamic_debug.h>
>  #include <linux/audit.h>
> +#include <linux/memory.h>
>  #include <uapi/linux/module.h>
>  #include "module-internal.h"
>  
> @@ -2181,6 +2182,9 @@ static void free_module(struct module *mod)
>  	synchronize_sched();
>  	mutex_unlock(&module_mutex);
>  
> +	/* Protect against patching of the module while it is being removed */
> +	mutex_lock(&text_mutex);
> +
>  	/* This may be empty, but that's OK */
>  	module_restore_mappings(&mod->init_layout);
>  	module_arch_freeing_init(mod);
> @@ -2194,6 +2198,7 @@ static void free_module(struct module *mod)
>  	/* Finally, free the core (containing the module structure) */
>  	module_restore_mappings(&mod->core_layout);
>  	module_memfree(mod->core_layout.base);
> +	mutex_unlock(&text_mutex);
>  }
>  
>  void *__symbol_get(const char *symbol)
> -- 
> 2.17.1
> 

Re: [PATCH v7 00/14] x86/alternative: text_poke() enhancements

[Peter Zijlstra]

On Tue, Dec 04, 2018 at 05:33:54PM -0800, Nadav Amit wrote:
> Which leads me to (b) - the patch-set is big "enough" IMHO. Indeed,
> there are open security issues in the kernel when it comes to W^X.  But
> some people would want to use Andy's temporary mm-struct for other uses.
> So additional security hardening may be left for future patches.

Yes, at the very least we should get the first 7 patches merged, since
they work and clean up the text poking irrespective of all that W^X
munging.

(also, I think you lost my ACK)

Re: [PATCH v7 00/14] x86/alternative: text_poke() enhancements

[Nadav Amit]

> On Dec 6, 2018, at 2:03 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Tue, Dec 04, 2018 at 05:33:54PM -0800, Nadav Amit wrote:
>> Which leads me to (b) - the patch-set is big "enough" IMHO. Indeed,
>> there are open security issues in the kernel when it comes to W^X.  But
>> some people would want to use Andy's temporary mm-struct for other uses.
>> So additional security hardening may be left for future patches.
> 
> Yes, at the very least we should get the first 7 patches merged, since
> they work and clean up the text poking irrespective of all that W^X
> munging.
> 
> (also, I think you lost my ACK)

Sorry for that. I will add.

But first, Thomas, Andy, are you ok with going with the first 7 patches?

IIRC, you are the one who asked to add the handling of modules, since it was
not clear whether some synchronization is needed after the poking (that is
done w/memcpy in this early stage).

I can add synchronization if needed until the rest of the series gets in.