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 D45E4C43217 for ; Thu, 1 Dec 2022 14:42:02 +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=1ImO0yd8cgZSoZePouzpPrQokNRrK5NZfdf12V7+Uhw=; b=P5t0WUoUq6AxS3 2axSfc9JvZE/EiFjbYy+ugWK2HvY9u2XegQGJX0yXmboz925EKIRR60jfX2Mxs+9D4rZNes3vzWUD QbE/NPYJRYnkpAit2Kx4aJxi53Lj1gs1WVrd2e9Aif3potIHlQdxyiucwvCcbXWTP0lvz4ytuzPus K9otDoK537NMRjjKAZfthdN5MiypntNtahZ7kASkOIFhMFyByd4CJfJfnVuXIc7vm/3iqkmzysoNt CdYHGODGYkCtGyeA+QQOyluBzoEfB/fN31GtbQ96arc314JyvHqgiaAgej0slm17wpgtkb/oXld7l UJZY9kB01SP+N4DM7FyA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p0kkA-007z08-1N; Thu, 01 Dec 2022 14:40:54 +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 1p0kk4-007ywK-Qd for linux-arm-kernel@lists.infradead.org; Thu, 01 Dec 2022 14:40:51 +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 901E8D6E; Thu, 1 Dec 2022 06:40:48 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.38.177]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7E98D3F67D; Thu, 1 Dec 2022 06:40:40 -0800 (PST) Date: Thu, 1 Dec 2022 14:40:34 +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: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221201_064049_000908_3183A263 X-CRM114-Status: GOOD ( 70.89 ) 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 On Thu, Dec 01, 2022 at 02:09:41PM +0100, Ard Biesheuvel wrote: > On Wed, 30 Nov 2022 at 18:45, Mark Rutland wrote: > > > > On Wed, Nov 30, 2022 at 03:26:19PM +0100, Ard Biesheuvel wrote: > > > On Wed, 30 Nov 2022 at 15:04, Mark Rutland wrote: > > > > 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. > > > > > > My reasoning was that if we are not going to return from it (in > > > return_to_handler()), we can rely on the interrupted function to > > > sign/authenticate it as usual. So the only reason for signing it here > > > is so that we can authenticate it in return_to_handler() if that > > > exists on the call path, removing a potentially vulnerable sequence > > > from that function. > > > > What I was trying to point out is that there is a window where this is spilled > > to the stack (and hence is potentially vulnerable) between > > ftrace_{caller,regs_caller}() and the end of ftrace_common(). > > > > So if we don't protect this when CONFIG_FUNCTION_GRAPH_TRACER=n, it could be > > clobbered during that window (e.g. while function tracers are invoked), > > *before* we return back into the instrumented function and sign the > > (potentially already clobbered) value. > > Agreed. > > But to clarify, the intent of this series is not to add protection to > ftrace, the intent is to get rid of the gadgets from the ftrace code > that can be abused even if you don't use ftrace at all. Ok; sorry for missing that; I'll need to think a little harder. > > Hence, my thinking is that we should sign this regardless of > > CONFIG_FUNCTION_GRAPH_TRACER to mitigate that case. I agree that we also want > > it to be signed while it's in the graph return stack (i.e. until the > > instrumented function returns back to return_to_handler()). In general, we > > should sign the value if it's going to be spilled to the stack. > > Sure, but it solves a different problem. Fair enough! I think we're agreed that something which solves both issues makes sense, even if that's not necessary for the gadgetisation issue specifically? > > > > 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. > > > > > > This is the reason for the xpaci instruction below. > > > > Unfortunately, that alone isn't sufficient. > > > > What I was alluding to is that this change means the ftrace graph return stack > > contains signed addresses, and other code doesn't expect that. For example, > > arm64's stacktrace code currently depends on the graph return stack containing > > plain pointers, and so that gets broken as of this patch when function graph > > tracing is enabled: > > > > | # uname -a > > | # Linux buildroot 6.1.0-rc7-00003-g44a67f0b8ac7 #1 SMP PREEMPT Wed Nov 30 17:19:38 GMT 2022 aarch64 GNU/Linux > > | # cat /proc/self/stack > > | [<0>] proc_pid_stack+0xc0/0x130 > > | [<0>] proc_single_show+0x68/0x120 > > | [<0>] seq_read_iter+0x16c/0x45c > > | [<0>] seq_read+0x98/0xd0 > > | [<0>] vfs_read+0xc8/0x2c0 > > | [<0>] ksys_read+0x78/0x110 > > | [<0>] __arm64_sys_read+0x24/0x30 > > | [<0>] invoke_syscall+0x50/0x120 > > | [<0>] el0_svc_common.constprop.0+0xd4/0xf4 > > | [<0>] do_el0_svc+0x34/0xd0 > > | [<0>] el0_svc+0x2c/0x84 > > | [<0>] el0t_64_sync_handler+0xf4/0x120 > > | [<0>] el0t_64_sync+0x18c/0x190 > > | # echo function_graph > /sys/kernel/debug/tracing/current_tracer > > | # cat /proc/self/stack > > | [<0>] 0xf5f98000083dff40 > > | [<0>] 0xd6b88000083e0f68 > > | [<0>] 0x21ac800008381ad0 > > | [<0>] 0xd0bc800008381e58 > > | [<0>] 0x22b280000834bc28 > > | [<0>] 0xf0ca80000834c5c8 > > | [<0>] 0x299080000834c684 > > | [<0>] 0xb1a1800008029cf0 > > | [<0>] 0x9bd0800008029e94 > > | [<0>] 0x1788800008029ee8 > > | [<0>] 0xa08680000916dd5c > > | [<0>] el0t_64_sync_handler+0xf4/0x120 > > | [<0>] el0t_64_sync+0x18c/0x190 > > > > That's unfortunate (and would break RELIABLE_STACKTRACE, which we're slowly > > getting towards being able to implement), but it's simple enough to account for > > in the stacktrace code. > > > > Indeed. Those functions should just strip the PAC bits, no? For that case, yup. That was roughly what I meant about it being simple to deal with in the stacktrace code. :) > > I have a fear that there are other cases where code tries to consume the graph > > return stack (or to match against entries within it), which would be similarly > > broken. I vaguely recall that we had issues of that shape in the past when we > > tried to adjust the reported PC value, and would need to go page that in to > > check that we don't open a similar issue here. > > OK FWIW, I'm happy to go audit that, I just wanted to make sure we didn't forget to do so, since it's not obvious that there are potential issues there. [...] > > > > > /* 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 > > > > 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. > > > > > > > > > > I think having all in one place makes it much easier to reason about, > > > tbh. Adding additional handling of the PAC state as well as the shadow > > > call stack in ftrace_graph_func() seems much more fiddly to me. > > > > I appreciate that concern, but my intuition here is the inverse; I'd like to > > avoid the conditionality in the regular tracing path to make that clearly > > balanced and (from my perspective) easier to reason about. > > > > I'm happy if we have to do a bit more work in ftrace_graph_func() and > > return_to_handler() since those are already more special anyway. > > > > Fair enough. As long as the asm routines have a SCS pop or AUTIASP > between reloading x30 and returning to it, I don't have any problems > with that. Sure; I think that's workable. I have a rough shape in mind, so I'll have a go at that as an example and try to get back to you shortly. With that in mind, I think we should also fix up qcom_link_stack_sanitisation(), since that ends up creating a gadget of the form: MOV X30, Xn RET ... and that can be fixed by leaving it to the compiler to save/restore x30, whereupon it should create a frame record and all the usual PAC goodness. Example patch below (reformatted into the usual arm64 inline asm style). Thanks, Mark. ---->8---- diff --git a/arch/arm64/kernel/proton-pack.c b/arch/arm64/kernel/proton-pack.c index bfce41c2a53b3..9fc54facf1ccb 100644 --- a/arch/arm64/kernel/proton-pack.c +++ b/arch/arm64/kernel/proton-pack.c @@ -250,12 +250,13 @@ static noinstr void qcom_link_stack_sanitisation(void) { u64 tmp; - asm volatile("mov %0, x30 \n" - ".rept 16 \n" - "bl . + 4 \n" - ".endr \n" - "mov x30, %0 \n" - : "=&r" (tmp)); + asm volatile( + " .rept 16 \n" + " bl . + 4 \n" + " .endr \n" + : "=&r" (tmp) + : + : "x30"); } static bp_hardening_cb_t spectre_v2_get_sw_mitigation_cb(void) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel