Message-ID: <20240305175846.qnyiru7uaa7itqba@treble>
Date: Tue, 5 Mar 2024 09:58:46 -0800
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Jiangfeng Xiao <xiaojiangfeng@...wei.com>
Cc: Kees Cook <keescook@...omium.org>, Jann Horn <jannh@...gle.com>,
	gustavoars@...nel.org, akpm@...ux-foundation.org,
	peterz@...radead.org, dave.hansen@...ux.intel.com,
	kirill.shutemov@...ux.intel.com, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org, linux-mm@...ck.org,
	nixiaoming@...wei.com, kepler.chenxin@...wei.com,
	wangbing6@...wei.com, wangfangpeng1@...wei.com,
	douzhaolei@...wei.com, Russell King <linux@...linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org,
	Ard Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH] usercopy: delete __noreturn from usercopy_abort

> >> For the usercopy_abort function, whether '__noreturn' is added
> >> does not affect the internal behavior of the usercopy_abort function.
> >> Therefore, it is recommended that '__noreturn' be deleted
> >> so that backtrace can work properly.
> > 
> > This isn't acceptable. Removing __noreturn this will break
> > objtool's processing of execution flow for livepatching, IBT, and
> > KCFI instrumentation. These all depend on an accurate control flow
> > descriptions, and usercopy_abort is correctly marked __noreturn.

__noreturn also has the benefit of enabling the compiler to produce more
compact code for callees.

> Thank you for providing this information.
> I'll go back to further understand how __noreturn is used
> in features such as KCFI and livepatching.

Adding ARM folks -- see
https://lkml.kernel.org/lkml/1709516385-7778-1-git-send-email-xiaojiangfeng@huawei.com
for the original bug report.

This is an off-by-one bug which is common in unwinders, due to the fact
that the address on the stack points to the return address rather than
the call address.

So, for example, when the last instruction of a function is a function
call (e.g., to a noreturn function), it can cause the unwinder to
incorrectly try to unwind from the function *after* the callee.

For ORC (x86), we fixed this by decrementing the PC for call frames (but
not exception frames).  I've seen user space unwinders do similar, for
non-signal frames.

Something like the following might fix your issue (completely untested):

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 360f0d2406bf..4891e38cdc1f 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -21,9 +21,7 @@ struct stackframe {
 	struct llist_node *kr_cur;
 	struct task_struct *tsk;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
 	bool ex_frame;
-#endif
 };
 
 static __always_inline
@@ -37,9 +35,8 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
 		frame->kr_cur = NULL;
 		frame->tsk = current;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
-		frame->ex_frame = in_entry_text(frame->pc);
-#endif
+		frame->ex_frame = !!regs;
+
 }
 
 extern int unwind_frame(struct stackframe *frame);
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 620aa82e3bdd..caed7436da09 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -154,9 +154,6 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task
 	frame->kr_cur = NULL;
 	frame->tsk = task;
 #endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
-	frame->ex_frame = in_entry_text(frame->pc);
-#endif
 }
 
 void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
@@ -167,6 +164,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 	if (regs) {
 		start_stack_trace(&frame, NULL, regs->ARM_fp, regs->ARM_sp,
 				  regs->ARM_lr, regs->ARM_pc);
+		frame.ex_frame = true;
 	} else if (task != current) {
 #ifdef CONFIG_SMP
 		/*
@@ -180,6 +178,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 				  thread_saved_sp(task), 0,
 				  thread_saved_pc(task));
 #endif
+		frame.ex_frame = false;
 	} else {
 here:
 		start_stack_trace(&frame, task,
@@ -187,6 +186,7 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 				  current_stack_pointer,
 				  (unsigned long)__builtin_return_address(0),
 				  (unsigned long)&&here);
+		frame.ex_frame = false;
 		/* skip this function */
 		if (unwind_frame(&frame))
 			return;
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 3bad79db5d6e..46a5b1eb3f0a 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
 	printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n",
 		loglvl, where, from);
 #elif defined CONFIG_BACKTRACE_VERBOSE
-	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
+	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n",
 		loglvl, where, (void *)where, from, (void *)from);
 #else
-	printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from);
+	printk("%s %ps from %pB\n", loglvl, (void *)where, (void *)from);
 #endif
 
 	if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 9d2192156087..99ded32196af 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -407,7 +407,7 @@ int unwind_frame(struct stackframe *frame)
 {
 	const struct unwind_idx *idx;
 	struct unwind_ctrl_block ctrl;
-	unsigned long sp_low;
+	unsigned long sp_low, pc;
 
 	/* store the highest address on the stack to avoid crossing it*/
 	sp_low = frame->sp;
@@ -417,19 +417,22 @@ int unwind_frame(struct stackframe *frame)
 	pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__,
 		 frame->pc, frame->lr, frame->sp);
 
-	idx = unwind_find_idx(frame->pc);
+	pc = frame->ex_frame ? frame->pc : frame->pc - 4;
+
+	idx = unwind_find_idx(pc);
 	if (!idx) {
-		if (frame->pc && kernel_text_address(frame->pc)) {
-			if (in_module_plt(frame->pc) && frame->pc != frame->lr) {
+		if (kernel_text_address(pc)) {
+			if (in_module_plt(pc) && frame->pc != frame->lr) {
 				/*
 				 * Quoting Ard: Veneers only set PC using a
 				 * PC+immediate LDR, and so they don't affect
 				 * the state of the stack or the register file
 				 */
 				frame->pc = frame->lr;
+				frame->ex_frame = false;
 				return URC_OK;
 			}
-			pr_warn("unwind: Index not found %08lx\n", frame->pc);
+			pr_warn("unwind: Index not found %08lx\n", pc);
 		}
 		return -URC_FAILURE;
 	}
@@ -442,7 +445,7 @@ int unwind_frame(struct stackframe *frame)
 	if (idx->insn == 1)
 		/* can't unwind */
 		return -URC_FAILURE;
-	else if (frame->pc == prel31_to_addr(&idx->addr_offset)) {
+	else if (frame->ex_frame && pc == prel31_to_addr(&idx->addr_offset)) {
 		/*
 		 * Unwinding is tricky when we're halfway through the prologue,
 		 * since the stack frame that the unwinder expects may not be
@@ -451,9 +454,10 @@ int unwind_frame(struct stackframe *frame)
 		 * a function, we are still effectively in the stack frame of
 		 * the caller, and the unwind info has no relevance yet.
 		 */
-		if (frame->pc == frame->lr)
+		if (pc == frame->lr)
 			return -URC_FAILURE;
 		frame->pc = frame->lr;
+		frame->ex_frame = false;
 		return URC_OK;
 	} else if ((idx->insn & 0x80000000) == 0)
 		/* prel31 to the unwind table */
@@ -515,6 +519,7 @@ int unwind_frame(struct stackframe *frame)
 	frame->lr = ctrl.vrs[LR];
 	frame->pc = ctrl.vrs[PC];
 	frame->lr_addr = ctrl.lr_addr;
+	frame->ex_frame = false;
 
 	return URC_OK;
 }
@@ -544,6 +549,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 		 */
 here:
 		frame.pc = (unsigned long)&&here;
+		frame.ex_frame = false;
 	} else {
 		/* task blocked in __switch_to */
 		frame.fp = thread_saved_fp(tsk);
@@ -554,11 +560,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 		 */
 		frame.lr = 0;
 		frame.pc = thread_saved_pc(tsk);
+		frame.ex_frame = false;
 	}
 
 	while (1) {
 		int urc;
-		unsigned long where = frame.pc;
+		unsigned long where = frame.ex_frame ? frame.pc : frame.pc - 4;
 
 		urc = unwind_frame(&frame);
 		if (urc < 0)

Powered by blists - more mailing lists