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 5A34AC4332F for ; Wed, 30 Nov 2022 14:06:36 +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=L8L+HlepgPUQXng6ndQLull3gXzaDlPfgkRMu01nxy0=; b=ZCA5/nFYgHPPuH Kjqw2HWvIMMOyYxfezTkxzIzA/Wfj5vvs4HtL7rQ50jYcEXC740zjkeE+KY4U7s+1MeRVbHlHJz5p GxUhPqRjobGjSuoIuNWLZPiD0xEGJlge8TLerZT9yeDRtac57upckqoQTLqA1EUYcM7twGYTSwl8v R0HkM21zA56k/X05YXUKtGa9hQeN4SUlO8uA76kH1l4EQxpZ3fshA+0eGGl0vmoqhWJH/861GjZlA zTdAlkO85NkVrpbK+0sP4T4wlJLRpRUGERhEwKsRxB0qumShtg8jaAsuS9RDkhvGVAT57lmW6aeCx XUJHHKMWodVdAZKcKymA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p0NiE-00GhcD-Qe; Wed, 30 Nov 2022 14:05:23 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p0NhI-00Ggqg-MG for linux-arm-kernel@lists.infradead.org; Wed, 30 Nov 2022 14:04:27 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E8CDDD6E; Wed, 30 Nov 2022 06:04:24 -0800 (PST) Received: from FVFF77S0Q05N.cambridge.arm.com (FVFF77S0Q05N.cambridge.arm.com [10.1.39.165]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4424C3F73D; Wed, 30 Nov 2022 06:04:17 -0800 (PST) Date: Wed, 30 Nov 2022 14:04:07 +0000 From: Mark Rutland To: Ard Biesheuvel Cc: linux-arm-kernel@lists.infradead.org, Marc Zyngier , Will Deacon , Kees Cook , Catalin Marinas , Mark Brown Subject: Re: [PATCH 4/4] arm64: ftrace: Add return address protection Message-ID: References: <20221129141803.1746898-1-ardb@kernel.org> <20221129141803.1746898-5-ardb@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20221129141803.1746898-5-ardb@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221130_060425_052047_3E4E6C46 X-CRM114-Status: GOOD ( 30.07 ) 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 Hi Ard, On Tue, Nov 29, 2022 at 03:18:03PM +0100, Ard Biesheuvel wrote: > Use the newly added asm macros to protect and restore the return address > in the ftrace call wrappers, based on whichever method is active (PAC > and/or shadow call stack). > > If the graph tracer is in use, this covers both the return address *to* > the ftrace call site as well as the return address *at* the call site, > and the latter will either be restored in return_to_handler(), or before > returning to the call site. > > Signed-off-by: Ard Biesheuvel > --- > arch/arm64/kernel/entry-ftrace.S | 28 +++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) As a heads-up, this code has recently changed quite significantly, and this won't apply to the version queued in arm64's for-next/{ftrace,core} branches. I had a direction of travel in mind with some changes for better stacktracing, which won't work with the approach here, so I'd prefer we do this a bit differently; more on that below. > diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S > index 795344ab4ec45889..c744e4dd8c90a352 100644 > --- a/arch/arm64/kernel/entry-ftrace.S > +++ b/arch/arm64/kernel/entry-ftrace.S > @@ -35,6 +35,11 @@ > * is missing from the LR and existing chain of frame records. > */ > .macro ftrace_regs_entry, allregs=0 > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + protect_return_address x9 > +#endif > + protect_return_address x30 I think if we're going to protect the callsite's original LR (x9 here), we should do that regardless of CONFIG_FUNCTION_GRAPH_TRACER; what matters is whether that's vulnerable rather than whether we intend to modify it, so I don't think it makes sene to protect it conditionally based on CONFIG_FUNCTION_GRAPH_TRACER. I'm a bit worried this might confuse some ftrace code manipulating the return address (e.g. manipulation of the ftrace graph return stack), as I don't think that's all PAC-clean, and might need some modification. > + > /* Make room for pt_regs, plus a callee frame */ > sub sp, sp, #(PT_REGS_SIZE + 16) > > @@ -89,7 +94,9 @@ SYM_CODE_START(ftrace_caller) > b ftrace_common > SYM_CODE_END(ftrace_caller) > > -SYM_CODE_START(ftrace_common) > +SYM_CODE_START_LOCAL(ftrace_common) > + alternative_insn nop, "xpaci x30", ARM64_HAS_ADDRESS_AUTH, IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) > + > sub x0, x30, #AARCH64_INSN_SIZE // ip (callsite's BL insn) > mov x1, x9 // parent_ip (callsite's LR) > ldr_l x2, function_trace_op // op > @@ -115,9 +122,27 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL) > ldr x30, [sp, #S_LR] > ldr x9, [sp, #S_PC] > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + /* grab the original return address from the stack */ > + ldr x10, [sp, #PT_REGS_SIZE + 8] > +#endif I'm planning to teach the stack unwinder how to unwind through ftrace_regs, such that we wouldn't need to duplicate the LR in a frame record here, and so we'd *only* have the copy inside the struct ftrace_regs. I think we don't need the copy here if we sign the callsite's LR against the base of the struct ftrace_regs. That way ftrace_graph_func() can sign the updated return address, and this code wouldn't need to care. The ftrace_regs have a copy of x18 that we can use to manipulate the SCS. > + > /* Restore the callsite's SP */ > add sp, sp, #PT_REGS_SIZE + 16 > > + restore_return_address x9 > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + /* compare the original return address with the actual one */ > + cmp x10, x30 > + b.ne 0f > + > + /* > + * If they are the same, unprotect it now. If it was modified, it will > + * be dealt with in return_to_handler() below. > + */ > + restore_return_address x30 > +0: > +#endif > ret x9 This means if the return address is clobbered, we'll blindly trust it without authentication, which IMO undermines the point of signing it in the first place. As above, I'd prefer that we had ftrace_graph_func() fix things up so that we can unconditionally authenticate things here, which would be a bit stronger and simpler to reason about. Thanks, Mark. > SYM_CODE_END(ftrace_common) > > @@ -329,6 +354,7 @@ SYM_CODE_START(return_to_handler) > ldp x6, x7, [sp, #48] > add sp, sp, #64 > > + restore_return_address x30 > ret > SYM_CODE_END(return_to_handler) > #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ > -- > 2.35.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel