[PATCH] bpf, arm64: Fix text_mutex critical section in bpf_arch_text_poke()

[Matthew Wood]

Widen the text_mutex critical section in bpf_arch_text_poke() to cover
the page permission changes made while updating a bpf prog's plt target.
When a long-jump trampoline is attached to or detached from a bpf prog,
the plt target at the end of the prog is currently updated before
text_mutex is taken, by temporarily making the page writable:

	set_memory_rw(page);
	WRITE_ONCE(plt->target, plt_target);
	set_memory_ro(page);

Leaving this outside text_mutex assumes the page belongs only to the
prog being poked, whose pokes are already serialized by its trampoline
lock. Commit 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory
management") broke that assumption; it switched the arm64 JIT to the bpf
prog pack allocator, which packs programs into small chunks of large
shared allocations, without updating bpf_arch_text_poke(). Since then,
unrelated progs (and their plts) commonly share a page. Two CPUs
attaching to or detaching from different target progs in the same page
hold different trampoline locks, so their permission changes can
interleave:

	CPU A                           CPU B
	set_memory_rw(page)
	                                set_memory_rw(page)
	                                WRITE_ONCE(plt_B->target, ...)
	                                set_memory_ro(page)
	WRITE_ONCE(plt_A->target, ...)  <- permission fault

This was hit on a 64K page arm64 machine:

  Unable to handle kernel write to read-only memory at virtual address ffff80008f46d5f8
  FSC = 0x0f: level 3 permission fault
  pte=00c00200ea5a0783
  pc : bpf_arch_text_poke+0x214/0x238
  Call trace:
   bpf_arch_text_poke+0x214/0x238 (P)
   __bpf_trampoline_link_prog+0x1c8/0x470
   bpf_trampoline_link_prog+0x64/0x90
   bpf_tracing_prog_attach+0x318/0x4a8
   bpf_raw_tp_link_attach+0x104/0x258
   bpf_raw_tracepoint_open+0x6c/0x90
   __sys_bpf+0x134c/0x3e10

Fixes: 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory management")
Signed-off-by: Matthew Wood <thepacketgeek@xxxxxxxxx>
---
 arch/arm64/net/bpf_jit_comp.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 475e70653454..66fab0bfd3da 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -3338,12 +3338,20 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type old_t,
 		 */
 		plt_target = (u64)&dummy_tramp;

+	/* pages of the bpf prog pack are shared between progs, so the
+	 * set_memory_rw()/set_memory_ro() window below must be serialized
+	 * against other pokers too.
+	 */
+	mutex_lock(&text_mutex);
+
 	if (plt_target) {
 		/* non-zero plt_target indicates we're patching a bpf prog,
 		 * which is read only.
 		 */
-		if (set_memory_rw(PAGE_MASK & ((uintptr_t)&plt->target), 1))
-			return -EFAULT;
+		if (set_memory_rw(PAGE_MASK & ((uintptr_t)&plt->target), 1)) {
+			ret = -EFAULT;
+			goto out;
+		}
 		WRITE_ONCE(plt->target, plt_target);
 		set_memory_ro(PAGE_MASK & ((uintptr_t)&plt->target), 1);
 		/* since plt target points to either the new trampoline
@@ -3357,10 +3365,11 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type old_t,
 	/* if the old target and the new target are both long jumps, no
 	 * patching is required
 	 */
-	if (old_insn == new_insn)
-		return 0;
+	if (old_insn == new_insn) {
+		ret = 0;
+		goto out;
+	}

-	mutex_lock(&text_mutex);
 	if (aarch64_insn_read(ip, &replaced)) {
 		ret = -EFAULT;
 		goto out;
--
2.53.0-Meta