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 X-Spam-Level: X-Spam-Status: No, score=-15.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5CEB2C49EA6 for ; Sat, 26 Jun 2021 15:36:43 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 2175F61C40 for ; Sat, 26 Jun 2021 15:36:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2175F61C40 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.microsoft.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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:Date: Message-ID:References:Cc:To:From:Subject:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=l2QT+lVRwYIjDaq0l7bZ3xJ/M1PDR+HWf0a/DaHUAuc=; b=ynMFNhOQkFfvZLT417bKafLFpV YUsCRTlLuaGlCe7Y39IRSu0IMB1lwY0YF65cBuB2HWULLjaNZEEALo6L11vCyCe02T77sjCde1f8b OA/ewrae0HJ/StnvU9EP4I+BpvNJhjY0lLYyZ6/wumHh6KM+GndZH17gbiid04N6e9M9GTAwb4+SS QtAlfbelh4cQImx1S/EJjs/x/U9/6IXsOOUGlEqfEdtg58E9lcbqL9Y4R1gDqciUSNlYat2zkKr9/ dm3reaFiLV6GZCJ0Ex1YFj8MHH/NzHB8jZtLARoPorXIfGJDRHAXdreYVPMk5fhNTYaeqRrSguSB7 KsFphuDA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lxAL0-004NB2-ME; Sat, 26 Jun 2021 15:35:18 +0000 Received: from linux.microsoft.com ([13.77.154.182]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lxAKw-004N9z-DJ for linux-arm-kernel@lists.infradead.org; Sat, 26 Jun 2021 15:35:16 +0000 Received: from [192.168.254.32] (unknown [47.187.214.213]) by linux.microsoft.com (Postfix) with ESMTPSA id B379320B6C50; Sat, 26 Jun 2021 08:35:10 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com B379320B6C50 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1624721711; bh=It/KkJfIsGta+X2x7LB/RSvqpRgJlj14fdfL2eXBuSE=; h=Subject:From:To:Cc:References:Date:In-Reply-To:From; b=MKlWLhoqqcHWZUN2qMeWfzkf8RRE2weSeCIxWeMHczmGwQGz17DkK5K+xh2BQI5A8 ScQV3w/Mkpi3lFq9LhAGYVpwN/5P9h3tukGl9dzXhuon/c3PBVte5U1MZutXiJ5G3C DWNVaAfuUBIWF7Ko7hhXKdbjzQOmbpNirzks8eos= Subject: Re: [RFC PATCH v5 1/2] arm64: Introduce stack trace reliability checks in the unwinder From: "Madhavan T. Venkataraman" To: Mark Rutland Cc: broonie@kernel.org, jpoimboe@redhat.com, ardb@kernel.org, nobuta.keiya@fujitsu.com, catalin.marinas@arm.com, will@kernel.org, jmorris@namei.org, pasha.tatashin@soleen.com, jthierry@redhat.com, linux-arm-kernel@lists.infradead.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org References: <20210526214917.20099-1-madvenka@linux.microsoft.com> <20210526214917.20099-2-madvenka@linux.microsoft.com> <20210624144021.GA17937@C02TD0UTHF1T.local> Message-ID: <1d01e3c9-fca5-06a8-7489-556fbe90d5b4@linux.microsoft.com> Date: Sat, 26 Jun 2021 10:35:09 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210626_083514_573728_9C7859AE X-CRM114-Status: GOOD ( 58.28 ) 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 I will send out the next version without frame->reliable as implementing a per-frame reliability thing obviously needs other changes and needs Mark Rutland's code reorg. Thanks! Madhavan On 6/25/21 10:39 AM, Madhavan T. Venkataraman wrote: > > > On 6/24/21 9:40 AM, Mark Rutland wrote: >> Hi Madhavan, >> >> On Wed, May 26, 2021 at 04:49:16PM -0500, madvenka@linux.microsoft.com wrote: >>> From: "Madhavan T. Venkataraman" >>> >>> The unwinder should check for the presence of various features and >>> conditions that can render the stack trace unreliable and mark the >>> the stack trace as unreliable for the benefit of the caller. >>> >>> Introduce the first reliability check - If a return PC is not a valid >>> kernel text address, consider the stack trace unreliable. It could be >>> some generated code. >>> >>> Other reliability checks will be added in the future. >>> >>> Signed-off-by: Madhavan T. Venkataraman >> >> At a high-level, I'm on-board with keeping track of this per unwind >> step, but if we do that then I want to be abel to use this during >> regular unwinds (e.g. so that we can have a backtrace idicate when a >> step is not reliable, like x86 does with '?'), and to do that we need to >> be a little more accurate. >> > > The only consumer of frame->reliable is livepatch. So, in retrospect, my > original per-frame reliability flag was an overkill. I was just trying to > provide extra per-frame debug information which is not really a requirement > for livepatch. > > So, let us separate the two. I will rename frame->reliable to frame->livepatch_safe. > This will apply to the whole stacktrace and not to every frame. > > Pass a livepatch_safe flag to start_backtrace(). This will be the initial value > of frame->livepatch_safe. So, if the caller knows that the starting frame is > unreliable, he can pass "false" to start_backtrace(). > > Whenever a reliability check fails, frame->livepatch_safe = false. After that > point, it will remain false till the end of the stacktrace. This keeps it simple. > > Also, once livepatch_safe is set to false, further reliability checks will not > be performed (what would be the point?). > > Finally, it might be a good idea to perform reliability checks even in > start_backtrace() so we don't assume that the starting frame is reliable even > if the caller passes livepatch_safe=true. What do you think? > >> I think we first need to do some more preparatory work for that, but >> regardless, I have some comments below. >> > > I agree that some more work is required to provide per-frame debug information > and tracking. That can be done later. It is not a requirement for livepatch. > >>> --- >>> arch/arm64/include/asm/stacktrace.h | 9 +++++++ >>> arch/arm64/kernel/stacktrace.c | 38 +++++++++++++++++++++++++---- >>> 2 files changed, 42 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h >>> index eb29b1fe8255..4c822ef7f588 100644 >>> --- a/arch/arm64/include/asm/stacktrace.h >>> +++ b/arch/arm64/include/asm/stacktrace.h >>> @@ -49,6 +49,13 @@ struct stack_info { >>> * >>> * @graph: When FUNCTION_GRAPH_TRACER is selected, holds the index of a >>> * replacement lr value in the ftrace graph stack. >>> + * >>> + * @reliable: Is this stack frame reliable? There are several checks that >>> + * need to be performed in unwind_frame() before a stack frame >>> + * is truly reliable. Until all the checks are present, this flag >>> + * is just a place holder. Once all the checks are implemented, >>> + * this comment will be updated and the flag can be used by the >>> + * caller of unwind_frame(). >> >> I'd prefer that we state the high-level semantic first, then drill down >> into detail, e.g. >> >> | @reliable: Indicates whether this frame is beleived to be a reliable >> | unwinding from the parent stackframe. This may be set >> | regardless of whether the parent stackframe was reliable. >> | >> | This is set only if all the following are true: >> | >> | * @pc is a valid text address. >> | >> | Note: this is currently incomplete. >> > > I will change the name of the flag. I will change the comment accordingly. > >>> */ >>> struct stackframe { >>> unsigned long fp; >>> @@ -59,6 +66,7 @@ struct stackframe { >>> #ifdef CONFIG_FUNCTION_GRAPH_TRACER >>> int graph; >>> #endif >>> + bool reliable; >>> }; >>> >>> extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame); >>> @@ -169,6 +177,7 @@ static inline void start_backtrace(struct stackframe *frame, >>> bitmap_zero(frame->stacks_done, __NR_STACK_TYPES); >>> frame->prev_fp = 0; >>> frame->prev_type = STACK_TYPE_UNKNOWN; >>> + frame->reliable = true; >>> } >> >> I think we need more data than this to be accurate. >> >> Consider arch_stack_walk() starting from a pt_regs -- the initial state >> (the PC from the regs) is accurate, but the first unwind from that will >> not be, and we don't account for that at all. >> >> I think we need to capture an unwind type in struct stackframe, which we >> can pass into start_backtrace(), e.g. >> > >> | enum unwind_type { >> | /* >> | * The next frame is indicated by the frame pointer. >> | * The next unwind may or may not be reliable. >> | */ >> | UNWIND_TYPE_FP, >> | >> | /* >> | * The next frame is indicated by the LR in pt_regs. >> | * The next unwind is not reliable. >> | */ >> | UNWIND_TYPE_REGS_LR, >> | >> | /* >> | * We do not know how to unwind to the next frame. >> | * The next unwind is not reliable. >> | */ >> | UNWIND_TYPE_UNKNOWN >> | }; >> >> That should be simple enough to set up around start_backtrace(), but >> we'll need further rework to make that simple at exception boundaries. >> With the entry rework I have queued for v5.14, we're *almost* down to a >> single asm<->c transition point for all vectors, and I'm hoping to >> factor the remainder out to C for v5.15, whereupon we can annotate that >> BL with some metadata for unwinding (with something similar to x86's >> UNWIND_HINT, but retained for runtime). >> > > I understood UNWIND_TYPE_FP and UNWIND_TYPE_REGS_LR. When would UNWIND_TYPE_UNKNOWN > be passed to start_backtrace? Could you elaborate? > > Regardless, the above comment applies only to per-frame tracking when it is eventually > implemented. For livepatch, it is not needed. At exception boundaries, if stack metadata > is available, then use that to unwind safely. Else, livepatch_safe = false. The latter > is what is being done in my patch series. So, we can go with that until stack metadata > becomes available. > > For the UNWIND_TYPE_REGS_LR and UNWIND_TYPE_UNKNOWN cases, the caller will > pass livepatch_safe=false to start_backtrace(). For UNWIND_TYPE_FP, the caller will > pass livepatch_safe=true. So, only UNWIND_TYPE_FP matters for livepatch. > >>> >>> #endif /* __ASM_STACKTRACE_H */ >>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >>> index d55bdfb7789c..9061375c8785 100644 >>> --- a/arch/arm64/kernel/stacktrace.c >>> +++ b/arch/arm64/kernel/stacktrace.c >>> @@ -44,21 +44,29 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) >>> unsigned long fp = frame->fp; >>> struct stack_info info; >>> >>> + frame->reliable = true; >> >> I'd prefer to do this the other way around, e.g. here do: >> >> | /* >> | * Assume that an unwind step is unreliable until it has passed >> | * all relevant checks. >> | */ >> | frame->reliable = false; >> >> ... then only set this to true once we're certain the step is reliable. >> >> That requires fewer changes below, and would also be more robust as if >> we forget to update this we'd accidentally mark an entry as unreliable >> rather than accidentally marking it as reliable. >> > > For livepatch_safe, the initial statement setting it to true at the > beginning of unwind_frame() goes away. But whenever a reliability check fails, > livepatch_safe has to be set to false. > >>> + >>> /* Terminal record; nothing to unwind */ >>> if (!fp) >>> return -ENOENT; >>> >>> - if (fp & 0xf) >>> + if (fp & 0xf) { >>> + frame->reliable = false; >>> return -EINVAL; >>> + } >>> >>> if (!tsk) >>> tsk = current; >>> >>> - if (!on_accessible_stack(tsk, fp, &info)) >>> + if (!on_accessible_stack(tsk, fp, &info)) { >>> + frame->reliable = false; >>> return -EINVAL; >>> + } >>> >>> - if (test_bit(info.type, frame->stacks_done)) >>> + if (test_bit(info.type, frame->stacks_done)) { >>> + frame->reliable = false; >>> return -EINVAL; >>> + } >>> >>> /* >>> * As stacks grow downward, any valid record on the same stack must be >>> @@ -74,8 +82,10 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) >>> * stack. >>> */ >>> if (info.type == frame->prev_type) { >>> - if (fp <= frame->prev_fp) >>> + if (fp <= frame->prev_fp) { >>> + frame->reliable = false; >>> return -EINVAL; >>> + } >>> } else { >>> set_bit(frame->prev_type, frame->stacks_done); >>> } >>> @@ -100,14 +110,32 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) >>> * So replace it to an original value. >>> */ >>> ret_stack = ftrace_graph_get_ret_stack(tsk, frame->graph++); >>> - if (WARN_ON_ONCE(!ret_stack)) >>> + if (WARN_ON_ONCE(!ret_stack)) { >>> + frame->reliable = false; >>> return -EINVAL; >>> + } >>> frame->pc = ret_stack->ret; >>> } >>> #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ >>> >>> frame->pc = ptrauth_strip_insn_pac(frame->pc); >>> >>> + /* >>> + * Check the return PC for conditions that make unwinding unreliable. >>> + * In each case, mark the stack trace as such. >>> + */ >>> + >>> + /* >>> + * Make sure that the return address is a proper kernel text address. >>> + * A NULL or invalid return address could mean: >>> + * >>> + * - generated code such as eBPF and optprobe trampolines >>> + * - Foreign code (e.g. EFI runtime services) >>> + * - Procedure Linkage Table (PLT) entries and veneer functions >>> + */ >>> + if (!__kernel_text_address(frame->pc)) >>> + frame->reliable = false; >> >> I don't think we should mention PLTs here. They appear in regular kernel >> text, and on arm64 they are generally not problematic for unwinding. The >> case in which they are problematic are where they interpose an >> trampoline call that isn't following the AAPCS (e.g. ftrace calls from a >> module, or calls to __hwasan_tag_mismatch generally), and we'll have to >> catch those explciitly (or forbid RELIABLE_STACKTRACE with HWASAN). >> > > I will remove the mention of PLTs. > >> >From a backtrace perspective, the PC itself *is* reliable, but the next >> unwind from this frame will not be, so I'd like to mark this as >> reliable and the next unwind as unreliable. We can do that with the >> UNWIND_TYPE_UNKNOWN suggestion above. >> > > In the livepatch_safe approach, it can be set to false as soon as the unwinder > realizes that there is unreliability, even if the unreliability is in the next > frame. Actually, this would avoid one extra unwind step for livepatch. > >> For the comment here, how about: >> >> | /* >> | * If the PC is not a known kernel text address, then we cannot >> | * be sure that a subsequent unwind will be reliable, as we >> | * don't know that the code follows our unwind requirements. >> | */ >> | if (!__kernel_text_address(frame-pc)) >> | frame->unwind = UNWIND_TYPE_UNKNOWN; >> > > OK. I can change the comment. > > Thanks! > > Madhavan > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel