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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9E3A8C54E41 for ; Tue, 5 Mar 2024 17:58:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F387C6B0081; Tue, 5 Mar 2024 12:58:52 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id EE8276B0082; Tue, 5 Mar 2024 12:58:52 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DB07E6B0085; Tue, 5 Mar 2024 12:58:52 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id C89B16B0081 for ; Tue, 5 Mar 2024 12:58:52 -0500 (EST) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id A17C81601D9 for ; Tue, 5 Mar 2024 17:58:52 +0000 (UTC) X-FDA: 81863746104.13.32F7D93 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf01.hostedemail.com (Postfix) with ESMTP id EEAC840002 for ; Tue, 5 Mar 2024 17:58:49 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=WyuOFdij; spf=pass (imf01.hostedemail.com: domain of jpoimboe@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=jpoimboe@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709661530; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=TlOX6/F8b6UkW3S3mSd4t8BdzdGGSllMNgb4BZ8Ecuo=; b=dgY4g0ifpeuuOX/wnoESG/DChGKS/RWGG5DvFzmdGVckBFu/JVZU5mM9cjkAElF24hCEnt xgkImW5oOPltqUVfSEfdVZcvyansszOdKgK/clFlzyp6aAO50D1pW6akLjYQGZa94u0Iri FJRPpKjKlGtDUFOwR0aFYy80CXcItZY= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709661530; a=rsa-sha256; cv=none; b=7XVKP5I0i2QdhbRYs206U7/u4x06DVN9rffbCivSssNxtTMM+yOcDYBn51hgUoITPxFwc8 WmBjKu7Wn1Url3pe2lxcuFSkJ51xn9XtCBf/q37P1rB8lor8Zkt6DlnmgWgNF//znk1/OK kqznQdPDwjCY0hoS+9oJXB+A7JkT18A= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=WyuOFdij; spf=pass (imf01.hostedemail.com: domain of jpoimboe@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=jpoimboe@kernel.org; dmarc=pass (policy=none) header.from=kernel.org 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-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Stat-Signature: fy7so3fufnn5bhgxu44be3x9j47t6w83 X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: EEAC840002 X-Rspam-User: X-HE-Tag: 1709661529-351111 X-HE-Meta: U2FsdGVkX18yi9KMcSvu0K/8yHsPpiCowyx86+XiRW+QC6+hmQWgCd9kUu6ufdS64gsmRdHAHX/xFmaAUDULJd/iqe4rOVwjcojdrhbdyva3VvIzdZa6jQr+6Rpi73MYTB38xCZCiveo9sjn8V4ftx5k8NQT40AN0F/s20L1KX7IBj5gIEhcAJ6TCXGS0Hoa5p8pE0BrsqzvEAB6JyLqz8YM2Hjzu0IHnnt3KTvCrIweXbyrqIUTVF2RAI0znuaoIiJQxBWSWDeGMFNAWvGxy63/dVsmi2OVCNjgHrX8ym5ZjJhdEgQFcDsy9vCwxqt90CrqaJD9qNpOWhfX2N46fXIFI61I84o9JH1gLeK3KDltcjDHRNgApUAQgd7RXQ/xWB9QriVivoUYzD2tWWu5EFByycPxnrKD1GdcO6pk0jVFayFrf/VuPk8Zu4hUQVhpAC9NqsPDuTee3gMtpJvGS3gdbPYWfm/r8/SQryldliuQnK56XIZ/Yi+Yp6uIvQZKZEDsFy362iyGO8PftaYhR1c21/KSVYbMtNmw7qnhz7U3149WIkigHNldMVCRT9NoNe8Ayy5wZG3CZIX5SRqt/OBKche8rPPLqUzJsdKJg7DB9iZGSDwzKFdENi07f9nBjKj6jmkOYxvIwbsIaF6sg/Q1XO7/z6xtKqjXaUNFk7zhqKyoUFoPU9zT+Z5bTZXPW4nDGpmwS7tL1xwGcer2rvedcFLlUAm3oAV+mhF2oZ2MtB/uz2NVnK1KiaiKocGmeDArorBmfP4DBUST17hxciIcqhgpa4PUMcDilvnd1By7S1GJGNGT5FJ68Vr3wCRhacmoT3UnnVGW968LMcHV0T/eVwShT72MxURimCcSDaC8yorDyg6cR6Gm/fnTQBLdOcoTAbOk9Ss2tnm1pKKiqe61fShFDbidcdGeMBRV8J9SEOIg4qfhTNm1M+MqSy0Qo4LnCe63ffuu0x/VllU ed4ZdiD+ 7lG9odDbBRiUcEm17aQuSE16gASgJOfPKACSc42D5LR6LZnek9Aal297ThgdBlAf1F2VMezE0REWG15O8v2jmjOk/jS4qOPws1OxMbTCZsNOnOvbNJsadtm+MQSLSEdx22Bmk6RxQBs2vUQI4ahTWhtEojgwactZfi0oi6ZVvjg24IcTFMiHLxlxCXiUawy86J0UBqbenfsCNy0bO6Fven+IOf+3Wpx0jOVH5Ec8Muu8LdcWTZOlMoFNTBarSeJYJ+Ym8vA9qU1cHbE4+mhOeOPCDa5Hdbpd9im6YcAshJqroMx/PYImWE/GDyw4X7Y+NtIPo X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: > >> 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)