From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 80B9BC54E41 for ; Tue, 5 Mar 2024 17:59:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=AqlrzrBqzI3R1tyYS+30AsepGxpy878Gs5rHSaJOMb8=; b=l+uw35TtGw8xHb xOzX/lU/zY13/DBKUmwwO1Hkp8pXU1kDI4IVEEPS7VQPPf7Ukr6gfYanf3YWiXupzrfD6KGjx/MkH 9Nb802yota7Qe7In9QoLTwlnwVwra8WsPCyKZwN+egYTAjv5favhXPKoKJm9F0RvuvRQUXIPWtf1Z hXYj0KQZKMLatnClzO3zOJbErE3LE4FAqt+gcHw14w/I7mJtqvk+oAQEZKmq47SkYOZuh2xk8iT9o djWp6tv/j5Udp/R0LTeOXnbGR0i5WIq9rNEsO8IAuLBEcfHL2Z4+Q4cvYdv17hLR2vrvgfqD2nQwU Pxfx9yy6V49QmeAbFACA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rhZ41-0000000EmBh-1gvT; Tue, 05 Mar 2024 17:58:53 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rhZ3y-0000000EmAV-0nIw for linux-arm-kernel@lists.infradead.org; Tue, 05 Mar 2024 17:58:51 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id E59E2616B7; Tue, 5 Mar 2024 17:58:48 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D08BBC433C7; Tue, 5 Mar 2024 17:58:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1709661528; bh=n/toLIPHbeaoCIED5/1fJO2jWwcxruDUgbg++Dsbijk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WyuOFdij78BbskVgyqWFM/kzuBAvozgJDjD+TyHF7hPV9/yM2i3UYBjxrJkgZGhdJ hW0lISaHBCuCz3x58q3LcNLqE0iGTM41jWlFKhXlgzZsm7d6YIn+vslueGkx6QnfSR GWWKGicIcHu12H3OYH9O6JyEOP+wleGX4Ylc32EUYlgbQ5v+9gp+D5Hkroim2V4IW2 wOij1bmoo7i0nbyfE4LownVfl/7lY+dZvCOeF3nG8tj5rWlBvW3fZPAawsdL81QtiU v2w6UlebkcHRtTMGiwSY+kewSqrytcnmtNwKciHGI3gOJ98z/1rPmc1zQ/HBtB3c7J IHyTn8DDq4gew== Date: Tue, 5 Mar 2024 09:58:46 -0800 From: Josh Poimboeuf To: Jiangfeng Xiao Cc: Kees Cook , Jann Horn , gustavoars@kernel.org, akpm@linux-foundation.org, peterz@infradead.org, dave.hansen@linux.intel.com, kirill.shutemov@linux.intel.com, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org, linux-mm@kvack.org, nixiaoming@huawei.com, kepler.chenxin@huawei.com, wangbing6@huawei.com, wangfangpeng1@huawei.com, douzhaolei@huawei.com, Russell King , linux-arm-kernel@lists.infradead.org, Ard Biesheuvel Subject: Re: [PATCH] usercopy: delete __noreturn from usercopy_abort Message-ID: <20240305175846.qnyiru7uaa7itqba@treble> References: <1709516385-7778-1-git-send-email-xiaojiangfeng@huawei.com> <202403040938.D770633@keescook> <77bb0d81-f496-7726-9495-57088a4c0bfc@huawei.com> <202403050129.5B72ACAA0D@keescook> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240305_095850_343485_926D3443 X-CRM114-Status: GOOD ( 30.54 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org > >> 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) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel