x86/alternative: text_poke() fixes

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 Jiri Kosina <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Kees Cook <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Fixes 9222f606506c ("x86/alternatives: Lockdep-enforce text_mutex in text_poke*()")

Suggested-by Peter Zijlstra <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 arch/x86/include/asm/text-patching.h |  3 +-
 arch/x86/kernel/alternative.c        | 72 +++++++++++++++++++++-------
 arch/x86/kernel/kgdb.c               | 15 ++++--
 3 files changed, 66 insertions(+), 24 deletions(-)

arch/x86/include/asm/text-patching.h +2 -1

index e85ff65c43c3..5a2600370763 100644
@@ -34,7 +34,8 @@ extern void *text_poke_early(void *addr, const void *opcode, size_t len);
  * On the local CPU you need to be protected again NMI or MCE handlers seeing an
  * inconsistent instruction while you patch.
  */
-extern void *text_poke(void *addr, const void *opcode, size_t len);
+extern int text_poke(void *addr, const void *opcode, size_t len);
+extern int 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;

arch/x86/kernel/alternative.c +54 -18

index ebeac487a20c..ebe9210dc92e 100644
@@ -678,23 +678,12 @@ 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 int __text_poke(void *addr, const void *opcode, size_t len)
 {
 	unsigned long flags;
 	char *vaddr;
 	struct page *pages[2];
-	int i;
+	int i, r = 0;
 
 	/*
 	 * While boot memory allocator is runnig we cannot use struct

@@ -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);

@@ -712,7 +699,8 @@ void *text_poke(void *addr, const void *opcode, size_t len)
 		WARN_ON(!PageReserved(pages[0]));
 		pages[1] = virt_to_page(addr + PAGE_SIZE);
 	}
-	BUG_ON(!pages[0]);
+	if (!pages[0])
+		return -EFAULT;
 	local_irq_save(flags);
 	set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[0]));
 	if (pages[1])

@@ -727,9 +715,57 @@ void *text_poke(void *addr, const void *opcode, size_t len)
 	/* 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]);
+		if (((char *)addr)[i] != ((char *)opcode)[i])
+			r = -EFAULT;
 	local_irq_restore(flags);
-	return addr;
+	return r;
+}
+
+/**
+ * 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.
+ */
+int text_poke(void *addr, const void *opcode, size_t len)
+{
+	int r;
+
+	lockdep_assert_held(&text_mutex);
+
+	r = __text_poke(addr, opcode, len);
+
+	/*
+	 * TODO: change the callers to consider the return value and remove this
+	 *       historical assertion.
+	 */
+	BUG_ON(r);
+
+	return r;
+}
+
+/**
+ * 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.
+ */
+int text_poke_kgdb(void *addr, const void *opcode, size_t len)
+{
+	return __text_poke(addr, opcode, len);
 }
 
 static void do_sync_core(void *info)

arch/x86/kernel/kgdb.c +10 -5

index 8e36f249646e..8091b2e381d4 100644
@@ -763,13 +763,15 @@ 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);
+	err = text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
+			     BREAK_INSTR_SIZE);
+	if (err)
+		return err;
 	err = probe_kernel_read(opc, (char *)bpt->bpt_addr, BREAK_INSTR_SIZE);
 	if (err)
 		return err;

@@ -788,12 +790,15 @@ 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);
+	err = text_poke_kgdb((void *)bpt->bpt_addr, bpt->saved_instr,
+			     BREAK_INSTR_SIZE);
+	if (err)
+		return err;
 	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;

Signature · 1 line

2.17.1

Quoted 12 lines

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.

Quoted 4 lines

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.

Hm, this looks reasonable and good to me.

Reviewed-by Masami Hiramatsu <[email protected]>

To: Ingo Molnar <[email protected]>
Cc: [email protected]>, [email protected]>, H. Peter Anvin <[email protected]>, Thomas Gleixner <[email protected]>, Borislav Petkov <[email protected]>, Dave Hansen <[email protected]>, Nadav Amit <[email protected]>, Andy Lutomirski <[email protected]>, Kees Cook <[email protected]>, Dave Hansen <[email protected]>, Masami Hiramatsu <[email protected]>
Subject: [PATCH v4 02/10] x86/jump_label: Use text_poke_early() during early init

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 <[email protected]>

Cc Kees Cook <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Co-Developed-by Peter Zijlstra <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
arch/x86/kernel/jump_label.c | 7 ++++++-

1 file changed, 6 insertions(+), 1 deletion(-)

arch/x86/kernel/jump_label.c +6 -1

index aac0c1f7e354..ed5fe274a7d8 100644
@@ -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) {

Signature · 1 line

2.17.1

arch/x86/mm/init_64.c +22 -0

+void __init poking_init(void)
+{
+	spinlock_t *ptl;
+	pte_t *ptep;
+
+	poking_mm = copy_init_mm();
+	if (!poking_mm) {
+		pr_err("x86/mm: error setting a separate poking address space");
+		return;
+	}
+
+	/*
+	 * 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;

To: Nadav Amit <[email protected]>
Cc: Ingo Molnar <[email protected]>, [email protected], [email protected], H. Peter Anvin <[email protected]>, Thomas Gleixner <[email protected]>, Borislav Petkov <[email protected]>, Dave Hansen <[email protected]>, Kees Cook <[email protected]>, Dave Hansen <[email protected]>
Subject: Re: [PATCH v4 05/10] x86/alternative: initializing temporary mm for patching

I don't seem to have gotten patches 0-2,7 for some reason; I'll try and dig them out of the LKML folder.

Quoted 22 lines · Nadav Amit

arch/x86/mm/init_64.c +22 -0

+void __init poking_init(void)
+{
+	spinlock_t *ptl;
+	pte_t *ptep;
+
+	poking_mm = copy_init_mm();
+	if (!poking_mm) {
+		pr_err("x86/mm: error setting a separate poking address space");
+		return;
+	}
+
+	/*
+	 * 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;

arch/x86/mm/init_64.c unsigned long memory_block_size_bytes(void) · line 1428

Quoted 10 lines

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

+
+	/*
+	 * 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);
+	if (!WARN_ON(!ptep))
+		pte_unmap_unlock(ptep, ptl);
+}

The difference in how we deal with -ENOMEM here is weird. I think we have a _lot_ of code that simply hard assumes we don't fail memory alloc on init.

I for instance would not mind to simply remove both branches and let the kernel crash and burn if we ever fail here.

From: Peter Zijlstra

Quoted 38 lines · Nadav Amit

Quoted 22 lines

arch/x86/mm/init_64.c +22 -0

+void __init poking_init(void)
+{
+	spinlock_t *ptl;
+	pte_t *ptep;
+
+	poking_mm = copy_init_mm();
+	if (!poking_mm) {
+		pr_err("x86/mm: error setting a separate poking address space");
+		return;
+	}
+
+	/*
+	 * 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;

arch/x86/mm/init_64.c unsigned long memory_block_size_bytes(void) · line 1428

Quoted 10 lines

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

+
+	/*
+	 * 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);
+	if (!WARN_ON(!ptep))
+		pte_unmap_unlock(ptep, ptl);
+}

The difference in how we deal with -ENOMEM here is weird. I think we have a _lot_ of code that simply hard assumes we don't fail memory alloc on init.

Quoted 2 lines

I for instance would not mind to simply remove both branches and let the kernel crash and burn if we ever fail here.

Actually, now that we removed the fallback of patching without poking_mm, a failure to allocate poking_mm should have had a BUG_ON().

For the second case, I think we still need either WARN_ON() or BUG_ON(), at least as some sort of an in-code comment. I’ll change it to BUG_ON() if you prefer.

arch/x86/kernel/alternative.c +17 -0

@@ -683,43 +684,108 @@ __ro_after_init unsigned long poking_addr;
 
 static int __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;
+	pte_t pte, *ptep;
+	spinlock_t *ptl;
+	int r = 0;
 
 	/*
+	 * 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);
+		if (cross_page_boundary)
+			pages[1] = vmalloc_to_page(addr + PAGE_SIZE);
 	} else {
 		pages[0] = virt_to_page(addr);
 		WARN_ON(!PageReserved(pages[0]));
+		if (cross_page_boundary)
+			pages[1] = virt_to_page(addr + PAGE_SIZE);
 	}
+
+	if (!pages[0] || (cross_page_boundary && !pages[1]))
 		return -EFAULT;
+
 	local_irq_save(flags);
+
+	/*

arch/x86/kernel/alternative.c +50 -0

+
+	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);
+	}
+
+	/*

To: Nadav Amit <[email protected]>
Cc: Ingo Molnar <[email protected]>, [email protected], [email protected], H. Peter Anvin <[email protected]>, Thomas Gleixner <[email protected]>, Borislav Petkov <[email protected]>, Dave Hansen <[email protected]>, Andy Lutomirski <[email protected]>, Kees Cook <[email protected]>, Dave Hansen <[email protected]>, Masami Hiramatsu <[email protected]>
Subject: Re: [PATCH v4 06/10] x86/alternative: use temporary mm for text poking

Quoted 40 lines · Nadav Amit

arch/x86/kernel/alternative.c +22 -0

@@ -683,43 +684,108 @@ __ro_after_init unsigned long poking_addr;
 
 static int __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;
+	pte_t pte, *ptep;
+	spinlock_t *ptl;
+	int r = 0;
 
 	/*
+	 * 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);
+		if (cross_page_boundary)
+			pages[1] = vmalloc_to_page(addr + PAGE_SIZE);
 	} else {
 		pages[0] = virt_to_page(addr);
 		WARN_ON(!PageReserved(pages[0]));
+		if (cross_page_boundary)
+			pages[1] = virt_to_page(addr + PAGE_SIZE);
 	}
+
+	if (!pages[0] || (cross_page_boundary && !pages[1]))
 		return -EFAULT;
+
 	local_irq_save(flags);
+
+	/*
+	 * The lock is not really needed, but this allows to avoid open-coding.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+
+	/*

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 727

Quoted 5 lines

arch/x86/kernel/alternative.c +5 -0

+	 * If we failed to allocate a PTE, fail. This should *never* happen,
+	 * since we preallocate the PTE.
+	 */
+	if (WARN_ON_ONCE(!ptep))
+		goto out;

Since we hard rely on init getting that right; can't we simply get rid of this?

Quoted 54 lines

arch/x86/kernel/alternative.c +54 -0

+
+	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.
+	 */

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 787

Quoted 6 lines

arch/x86/kernel/alternative.c +6 -0

+	unuse_temporary_mm(prev);
+
+	pte_unmap_unlock(ptep, ptl);
+out:
+	if (memcmp(addr, opcode, len))
+		r = -EFAULT;

How could this ever fail? And how can we reliably recover from that?

I mean, we can move that BUG_ON() we have in text_poke() down a level, but for example the static_key/jump_label code has no real option on failing this.

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 791

Quoted 4 lines

arch/x86/kernel/alternative.c +1 -0

+
 	local_irq_restore(flags);
 	return r;
 }

Other than that, this looks really good!

Quoted 6 lines

Quoted 6 lines

arch/x86/kernel/alternative.c +6 -0

+	/*
+	 * The lock is not really needed, but this allows to avoid open-coding.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+
+	/*

Quoted 8 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 727

Quoted 5 lines

arch/x86/kernel/alternative.c +5 -0

+	 * If we failed to allocate a PTE, fail. This should *never* happen,
+	 * since we preallocate the PTE.
+	 */
+	if (WARN_ON_ONCE(!ptep))
+		goto out;

Since we hard rely on init getting that right; can't we simply get rid of this?

This is a repeated complaint of yours, which I do not feel comfortable with. One day someone will run some static analysis tool and start finding that all these checks are missing.

The question is why do you care about them.

If it is because they affect the
generated code and make it less efficient, I can fully understand and perhaps we should have something like PARANOID_WARN_ON_ONCE() which compiles into nothing unless a certain debug option is set.

If it is about the way the source code looks - I guess it doesn’t sore my eyes as hard as some other stuff, and I cannot do much about it (other than removing it as you asked).

Quoted 5 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 787

Quoted 3 lines

arch/x86/kernel/alternative.c +3 -0

+out:
+	if (memcmp(addr, opcode, len))
+		r = -EFAULT;

How could this ever fail? And how can we reliably recover from that?

This code has been there before (with slightly uglier code). Before this patch, a BUG_ON() was used here. However, I noticed that kgdb actually checks that text_poke() succeeded after calling it and gracefully fail. However, this was useless, since text_poke() would panic before kgdb gets the chance to do anything (see patch 7).

To: Nadav Amit <[email protected]>
Cc: Ingo Molnar <[email protected]>, LKML <[email protected]>, X86 ML <[email protected]>, H. Peter Anvin <[email protected]>, Thomas Gleixner <[email protected]>, Borislav Petkov <[email protected]>, Dave Hansen <[email protected]>, Andy Lutomirski <[email protected]>, Kees Cook <[email protected]>, Dave Hansen <[email protected]>, Masami Hiramatsu <[email protected]>
Subject: Re: [PATCH v4 06/10] x86/alternative: use temporary mm for text poking

Quoted 15 lines · Nadav Amit

Quoted 6 lines

Quoted 6 lines

arch/x86/kernel/alternative.c +6 -0

+	/*
+	 * The lock is not really needed, but this allows to avoid open-coding.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+
+	/*

Quoted 8 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 727

Quoted 5 lines

arch/x86/kernel/alternative.c +5 -0

+	 * If we failed to allocate a PTE, fail. This should *never* happen,
+	 * since we preallocate the PTE.
+	 */
+	if (WARN_ON_ONCE(!ptep))
+		goto out;

Since we hard rely on init getting that right; can't we simply get rid of this?

Quoted 5 lines

This is a repeated complaint of yours, which I do not feel comfortable with. One day someone will run some static analysis tool and start finding that all these checks are missing.

The question is why do you care about them.

Mostly because they should not be happening, ever. And if they happen, there really isn't anything sensible we can do about it.

Quoted 5 lines

If it is because they affect the
generated code and make it less efficient, I can fully understand and perhaps we should have something like PARANOID_WARN_ON_ONCE() which compiles into nothing unless a certain debug option is set.

Quoted 3 lines

If it is about the way the source code looks - I guess it doesn’t sore my eyes as hard as some other stuff, and I cannot do much about it (other than removing it as you asked).

And yes on the above two points. It adds both runtime overhead (albeit trivially small) and code complexity.

Quoted 11 lines

Quoted 5 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 787

Quoted 3 lines

arch/x86/kernel/alternative.c +3 -0

+out:
+	if (memcmp(addr, opcode, len))
+		r = -EFAULT;

How could this ever fail? And how can we reliably recover from that?

This code has been there before (with slightly uglier code). Before this patch, a BUG_ON() was used here. However, I noticed that kgdb actually checks that text_poke() succeeded after calling it and gracefully fail. However, this was useless, since text_poke() would panic before kgdb gets the chance to do anything (see patch 7).

Yes, I know it was there before, and I did see kgdb do it too. But aside from that out-label case, which we also should never hit, how can we realistically ever fail that memcmp()?

If we fail here, something is _seriously_ buggered.

Quoted 16 lines · Nadav Amit

Quoted 15 lines

Quoted 15 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 727

Quoted 12 lines

arch/x86/kernel/alternative.c +11 -0

+	/*
+	 * The lock is not really needed, but this allows to avoid open-coding.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+
+	/*
+	 * If we failed to allocate a PTE, fail. This should *never* happen,
+	 * since we preallocate the PTE.
+	 */
+	if (WARN_ON_ONCE(!ptep))
+		goto out;

Since we hard rely on init getting that right; can't we simply get rid of this?

I understand. So the question is - what would you prefer: something like PARANOID_WARN_ON_ONCE() or should I just remove the assertion?

Quoted 12 lines

Quoted 11 lines

Quoted 5 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 787

Quoted 3 lines

arch/x86/kernel/alternative.c +3 -0

+out:
+	if (memcmp(addr, opcode, len))
+		r = -EFAULT;

How could this ever fail? And how can we reliably recover from that?

This code has been there before (with slightly uglier code). Before this patch, a BUG_ON() was used here. However, I noticed that kgdb actually checks that text_poke() succeeded after calling it and gracefully fail. However, this was useless, since text_poke() would panic before kgdb gets the chance to do anything (see patch 7).

Quoted 5 lines

Yes, I know it was there before, and I did see kgdb do it too. But aside from that out-label case, which we also should never hit, how can we realistically ever fail that memcmp()?

If we fail here, something is _seriously_ buggered.

I agree. But as it may be useful at least to warn in such a case, as debugging of SMC/CMC is hard. For example, if there is some sort of a race between module (un)loading and static-keys - such a check might be beneficial to indicate so. Having said that, changing it into VM_BUG_ON() or something similar may make more sense.

Personally, I don’t care much - I’m just worried that I made some intrusive changes *and* you want me to remove the assertion that checks that I didn’t screw up.

Quoted 14 lines · Nadav Amit

Quoted 14 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 727

Quoted 11 lines

arch/x86/kernel/alternative.c +11 -0

+	/*
+	 * The lock is not really needed, but this allows to avoid open-coding.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+
+	/*
+	 * If we failed to allocate a PTE, fail. This should *never* happen,
+	 * since we preallocate the PTE.
+	 */
+	if (WARN_ON_ONCE(!ptep))
+		goto out;

Since we hard rely on init getting that right; can't we simply get rid of this?

Quoted 3 lines

If it is about the way the source code looks - I guess it doesn’t sore my eyes as hard as some other stuff, and I cannot do much about it (other than removing it as you asked).

Quoted 15 lines · Nadav Amit

Quoted 14 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 727

Quoted 11 lines

arch/x86/kernel/alternative.c +11 -0

+	/*
+	 * The lock is not really needed, but this allows to avoid open-coding.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+
+	/*
+	 * If we failed to allocate a PTE, fail. This should *never* happen,
+	 * since we preallocate the PTE.
+	 */
+	if (WARN_ON_ONCE(!ptep))
+		goto out;

Since we hard rely on init getting that right; can't we simply get rid of this?

Quoted 5 lines

This is a repeated complaint of yours, which I do not feel comfortable with. One day someone will run some static analysis tool and start finding that all these checks are missing.

The question is why do you care about them.

Mostly because they should not be happening, ever.

[...] And if they happen, there really isn't anything sensible we can do about it.

Quoted 8 lines

If it is because they affect the
generated code and make it less efficient, I can fully understand and perhaps we should have something like PARANOID_WARN_ON_ONCE() which compiles into nothing unless a certain debug option is set.

If it is about the way the source code looks - I guess it doesn’t sore my eyes as hard as some other stuff, and I cannot do much about it (other than removing it as you asked).

And yes on the above two points. It adds both runtime overhead (albeit trivially small) and code complexity.

Quoted 11 lines

Quoted 5 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 787

Quoted 3 lines

arch/x86/kernel/alternative.c +3 -0

+out:
+	if (memcmp(addr, opcode, len))
+		r = -EFAULT;

How could this ever fail? And how can we reliably recover from that?

This code has been there before (with slightly uglier code). Before this patch, a BUG_ON() was used here. However, I noticed that kgdb actually checks that text_poke() succeeded after calling it and gracefully fail. However, this was useless, since text_poke() would panic before kgdb gets the chance to do anything (see patch 7).

Yes, I know it was there before, and I did see kgdb do it too. But aside from that out-label case, which we also should never hit, how can we realistically ever fail that memcmp()?

If we fail here, something is _seriously_ buggered.

Quoted 21 lines · Peter Zijlstra

Quoted 19 lines · Nadav Amit

Quoted 6 lines

Quoted 6 lines

arch/x86/kernel/alternative.c +6 -0

+	/*
+	 * The lock is not really needed, but this allows to avoid open-coding.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+
+	/*

Quoted 8 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 727

Quoted 5 lines

arch/x86/kernel/alternative.c +5 -0

+	 * If we failed to allocate a PTE, fail. This should *never* happen,
+	 * since we preallocate the PTE.
+	 */
+	if (WARN_ON_ONCE(!ptep))
+		goto out;

Since we hard rely on init getting that right; can't we simply get rid of this?

This is a repeated complaint of yours, which I do not feel comfortable with. One day someone will run some static analysis tool and start finding that all these checks are missing.

Quoted 3 lines

Quoted 1 line

The question is why do you care about them.

Mostly because they should not be happening, ever.

Since get_locked_pte() might in principle return NULL, it's an entirely routine pattern to check the return for NULL. This will save reviewer time in the future.

Quoted 9 lines

Quoted 8 lines

If it is because they affect the
generated code and make it less efficient, I can fully understand and perhaps we should have something like PARANOID_WARN_ON_ONCE() which compiles into nothing unless a certain debug option is set.

If it is about the way the source code looks - I guess it doesn’t sore my eyes as hard as some other stuff, and I cannot do much about it (other than removing it as you asked).

Quoted 2 lines

And yes on the above two points. It adds both runtime overhead (albeit trivially small) and code complexity.

It's trivially small cycle level overhead in something that will be burdened by two TLB flushes anyway is is utterly slow.

Quoted 12 lines

Quoted 11 lines

Quoted 5 lines

arch/x86/kernel/alternative.c __ro_after_init unsigned long poking_addr; · line 787

Quoted 3 lines

arch/x86/kernel/alternative.c +3 -0

+out:
+	if (memcmp(addr, opcode, len))
+		r = -EFAULT;

How could this ever fail? And how can we reliably recover from that?

This code has been there before (with slightly uglier code). Before this patch, a BUG_ON() was used here. However, I noticed that kgdb actually checks that text_poke() succeeded after calling it and gracefully fail. However, this was useless, since text_poke() would panic before kgdb gets the chance to do anything (see patch 7).

Quoted 5 lines

Yes, I know it was there before, and I did see kgdb do it too. But aside from that out-label case, which we also should never hit, how can we realistically ever fail that memcmp()?

If we fail here, something is _seriously_ buggered.

So wouldn't it be better to just document and verify our assumptions of this non-trivial code by using return values intelligently?

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 <[email protected]>

Cc Kees Cook <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 arch/x86/include/asm/fixmap.h |   2 -
 arch/x86/kernel/alternative.c | 112 +++++++++++++++++++++++++++-------
 2 files changed, 89 insertions(+), 25 deletions(-)

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

index 50ba74a34a37..9da8cccdf3fb 100644

arch/x86/include/asm/fixmap.h +0 -2

-	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

arch/x86/kernel/alternative.c +89 -23

index d3ae5c26e5a0..96607ef285c3 100644
@@ -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,43 +684,108 @@ __ro_after_init unsigned long poking_addr;
 
 static int __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, r = 0;
+	pte_t pte, *ptep;
+	spinlock_t *ptl;
+	int r = 0;
 
 	/*
-	 * 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);
 	}
-	if (!pages[0])
+
+	if (!pages[0] || (cross_page_boundary && !pages[1]))
 		return -EFAULT;
+
 	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++)
-		if (((char *)addr)[i] != ((char *)opcode)[i])
-			r = -EFAULT;
+
+	/*
+	 * The lock is not really needed, but this allows to avoid open-coding.
+	 */
+	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
+
+	/*
+	 * If we failed to allocate a PTE, fail. This should *never* happen,
+	 * since we preallocate the PTE.
+	 */
+	if (WARN_ON_ONCE(!ptep))
+		goto out;
+
+	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);
+out:
+	if (memcmp(addr, opcode, len))
+		r = -EFAULT;
+
 	local_irq_restore(flags);
 	return r;
 }

Signature · 1 line

2.17.1

To: Nadav Amit <[email protected]>
Cc: Ingo Molnar <[email protected]>, [email protected]>, [email protected]>, H. Peter Anvin <[email protected]>, Thomas Gleixner <[email protected]>, Borislav Petkov <[email protected]>, Dave Hansen <[email protected]>, Andy Lutomirski <[email protected]>, Kees Cook <[email protected]>, Peter Zijlstra <[email protected]>, Dave Hansen <[email protected]>, Masami Hiramatsu <[email protected]>
Subject: Re: [PATCH v4 06/10] x86/alternative: use temporary mm for text poking

Quoted 31 lines · 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 <[email protected]>

Cc Kees Cook <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
arch/x86/include/asm/fixmap.h | 2 -
arch/x86/kernel/alternative.c | 112 +++++++++++++++++++++++++++-------

2 files changed, 89 insertions(+), 25 deletions(-)

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

index 50ba74a34a37..9da8cccdf3fb 100644

arch/x86/include/asm/fixmap.h enum fixed_addresses { · line 105

Quoted 3 lines

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

@@ -103,8 +103,6 @@ enum fixed_addresses {
 #ifdef CONFIG_PARAVIRT
 	FIX_PARAVIRT_BOOTMAP,
 #endif

Hello Nadav,

with the remove of FIX_TEXT_POKE1 and FIX_TEXT_POKE0 i get the following build error:

/home/damian/kernel/linux/arch/x86/xen/mmu_pv.c:2321:7: Fehler: »FIX_TEXT_POKE0« nicht deklariert (erstmalige Verwendung in dieser Funktion); meinten Sie »FIX_TBOOT_BASE«?
case FIX_TEXT_POKE0:

      ^~~~~~~~~~~~~~
      FIX_TBOOT_BASE
/home/damian/kernel/linux/arch/x86/xen/mmu_pv.c:2321:7: Anmerkung: jeder nicht deklarierte Bezeichner wird nur einmal für jede Funktion, in der er vorkommt, gemeldet
/home/damian/kernel/linux/arch/x86/xen/mmu_pv.c:2322:7: Fehler: »FIX_TEXT_POKE1« nicht deklariert (erstmalige Verwendung in dieser Funktion); meinten Sie »FIX_TBOOT_BASE«?
 case FIX_TEXT_POKE1:
      ^~~~~~~~~~~~~~
      FIX_TBOOT_BASE

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.

To: Nadav Amit <[email protected]>
Cc: Ingo Molnar <[email protected]>, [email protected], [email protected], H. Peter Anvin <[email protected]>, Thomas Gleixner <[email protected]>, Borislav Petkov <[email protected]>, Dave Hansen <[email protected]>, Andy Lutomirski <[email protected]>, Kees Cook <[email protected]>, Dave Hansen <[email protected]>, Masami Hiramatsu <[email protected]>
Subject: Re: [PATCH v4 09/10] x86/jump-label: remove support for custom poker

Quoted 3 lines · 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.

Right; we used to have a 3rd way, but that is long gone.

Nice cleanup!