Message ID | 20230615193722.194131053@infradead.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp887650vqr; Thu, 15 Jun 2023 13:08:08 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4K/Q9qrBxQSmte3R42s/5qWlaDLX0aynPQxzj1gK2qO7vDWokwDS882fIKkHDLrqiWfbAK X-Received: by 2002:a17:907:6e92:b0:982:6bba:79ce with SMTP id sh18-20020a1709076e9200b009826bba79cemr136730ejc.12.1686859687890; Thu, 15 Jun 2023 13:08:07 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1686859687; cv=none; d=google.com; s=arc-20160816; b=m9mch22d+su7s/gOY0TAk2HyZFAFtk/tLfAldLFjKxj6r+150TgTnSgUSndSHyM2oh CQKNsYsKH7L9oE20Wh8saPs/02TctBfFvpIG2DErcyUB+iZAXOy+dM3pcZVe2qaiuWth xxP+4W722RJ+KyOgn8V2p6qeqFxz4h11LlW36YO+1ljqurZbaPaGNRxdg89Ro6QMsBmh B9ly8jXBmucovmZ1MpMzrOQP65p5cqEfjc0IY0dCRfgxr9Iq/tpZN2JahSFoZBJ6Ijvs nkTTAgbwHPG94nYNMKwSQwDv72FR+jsmwlgB+Y+D8Nu8o52r0cS6cAxWnus5EKlJv6rr KjhA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:references:subject:cc:to:from:date :user-agent:message-id:dkim-signature; bh=5QR/G+Po7KVXkZ8pw8MI2J3h+jcGvJpJMtnZs0VY6Uw=; b=OdlXaxw2M2SHx6nuyW/Q4pltjWl1CfFD302PXI+HgwjLz6fTRFj0uSm158W37wlPIv f22IwQQZMTvMtP93nlBOYxCVgh+ABSy0NnDDehjsktxI7AjLlhVurqR9jNEO755aQXa3 UptdSpIf+gkF3aOKK8W/SmGd40oZjFMpWif8my0I9vmIvpitky3x97/bPPiP33Bs+XKw tPioxy/U40v/dV2edFYuwYX06wHJc60yQgacZJz55WVkwsccQ9BhQTxUkbXyP46ALyAT BilK36LjfIOo6VKf8/JLgFundw1DfWdvTMRslOY+rQLLk8Kk+aYa6Osabx4Tv2P9U0Se nJbQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@infradead.org header.s=desiato.20200630 header.b=VH+MQ2ph; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id q19-20020a1709066b1300b0097455b9d069si10284359ejr.1033.2023.06.15.13.07.41; Thu, 15 Jun 2023 13:08:07 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@infradead.org header.s=desiato.20200630 header.b=VH+MQ2ph; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231738AbjFOTku (ORCPT <rfc822;maxin.john@gmail.com> + 99 others); Thu, 15 Jun 2023 15:40:50 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56826 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229572AbjFOTkn (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 15 Jun 2023 15:40:43 -0400 Received: from desiato.infradead.org (desiato.infradead.org [IPv6:2001:8b0:10b:1:d65d:64ff:fe57:4e05]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0724B2953 for <linux-kernel@vger.kernel.org>; Thu, 15 Jun 2023 12:40:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; h=Content-Type:MIME-Version:References: Subject:Cc:To:From:Date:Message-ID:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:In-Reply-To; bh=5QR/G+Po7KVXkZ8pw8MI2J3h+jcGvJpJMtnZs0VY6Uw=; b=VH+MQ2phJAjRHH/4i2FQnrxJXJ Of7AucP+aR7hJKYULJXBW0cOEdrBvHqOiYedgFVW3NEmFTRF6gYU/6Z4VXDhaSEHsV9Oxop2SBzZa Txm9EE9XsckO/OjI7qy0ID7RJnAVBWX4dQEWjbxuaoKvngR39LRNmBIGV8vK+ISJtY0/dfrqoxFWI ZOjeicpPcAjtcdc9s5AS46W7bjzGwDmnD/8y93CdPyQi4EzgDPRy37QR9CIbR7XshqofHeqZzq/4U iosEOH2iSmO1XhZ1YTF8iamxZyFGPJo9rvgzJlwMD0VqkLvIZwZkrNpzOUZb2y8n5+IQWEG6z25VH aqWvcIwg==; Received: from j130084.upc-j.chello.nl ([24.132.130.84] helo=noisy.programming.kicks-ass.net) by desiato.infradead.org with esmtpsa (Exim 4.96 #2 (Red Hat Linux)) id 1q9spY-00BtUU-1s; Thu, 15 Jun 2023 19:40:29 +0000 Received: from hirez.programming.kicks-ass.net (hirez.programming.kicks-ass.net [192.168.1.225]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by noisy.programming.kicks-ass.net (Postfix) with ESMTPS id 14AA73003E1; Thu, 15 Jun 2023 21:40:27 +0200 (CEST) Received: by hirez.programming.kicks-ass.net (Postfix, from userid 0) id EC6C52461D7D2; Thu, 15 Jun 2023 21:40:26 +0200 (CEST) Message-ID: <20230615193722.194131053@infradead.org> User-Agent: quilt/0.66 Date: Thu, 15 Jun 2023 21:35:48 +0200 From: Peter Zijlstra <peterz@infradead.org> To: x86@kernel.org, alyssa.milburn@linux.intel.com Cc: linux-kernel@vger.kernel.org, peterz@infradead.org, samitolvanen@google.com, keescook@chromium.org, jpoimboe@kernel.org, joao@overdrivepizza.com, tim.c.chen@linux.intel.com Subject: [PATCH 2/2] x86/fineibt: Poison ENDBR at +0 References: <20230615193546.949657149@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1768800583727399784?= X-GMAIL-MSGID: =?utf-8?q?1768800583727399784?= |
Series |
x86/cfi: Fix FineIBT
|
|
Commit Message
Peter Zijlstra
June 15, 2023, 7:35 p.m. UTC
Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
booting on IBT enabled hardware obtain FineIBT, the indirect functions
look like:
__cfi_foo:
endbr64
subl $hash, %r10d
jz 1f
ud2
nop
1:
foo:
endbr64
This is because clang currently does not supress ENDBR emission for
functions it provides a __cfi prologue symbol for.
Having this second ENDBR however makes it possible to elide the CFI
check. Therefore, we should poison this second ENDBR (if present) when
switching to FineIBT mode.
Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
Reported-by: "Milburn, Alyssa" <alyssa.milburn@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/kernel/alternative.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
Comments
On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote: > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and > booting on IBT enabled hardware obtain FineIBT, the indirect functions > look like: > > __cfi_foo: > endbr64 > subl $hash, %r10d > jz 1f > ud2 > nop > 1: > foo: > endbr64 > > This is because clang currently does not supress ENDBR emission for > functions it provides a __cfi prologue symbol for. Should this be considered a bug in Clang? > > Having this second ENDBR however makes it possible to elide the CFI > check. Therefore, we should poison this second ENDBR (if present) when > switching to FineIBT mode. > > Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT") > Reported-by: "Milburn, Alyssa" <alyssa.milburn@intel.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Looks like a good work-around. Acked-by: Kees Cook <keescook@chromium.org>
On Tue, Jun 20, 2023 at 2:55 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote: > > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and > > booting on IBT enabled hardware obtain FineIBT, the indirect functions > > look like: > > > > __cfi_foo: > > endbr64 > > subl $hash, %r10d > > jz 1f > > ud2 > > nop > > 1: > > foo: > > endbr64 > > > > This is because clang currently does not supress ENDBR emission for > > functions it provides a __cfi prologue symbol for. > > Should this be considered a bug in Clang? The endbr is needed with KCFI if we have FineIBT disabled for some reason. There's some discussion here: https://github.com/ClangBuiltLinux/linux/issues/1735 However, since the kernel handles FineIBT patching, it might be cleaner to let it also poison the extra endbr. Sami
On Tue, Jun 20, 2023 at 02:55:10PM -0700, Kees Cook wrote: > On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote: > > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and > > booting on IBT enabled hardware obtain FineIBT, the indirect functions > > look like: > > > > __cfi_foo: > > endbr64 > > subl $hash, %r10d > > jz 1f > > ud2 > > nop > > 1: > > foo: > > endbr64 > > > > This is because clang currently does not supress ENDBR emission for > > functions it provides a __cfi prologue symbol for. > > Should this be considered a bug in Clang? No, I don't think so. I was going to say this is perhaps insufficiently explored space, but upon more consideration I think this is actually correct behaviour (and I need to write a better Changelog). The issue is that the compiler generates code for kCFI+IBT, it doesn't know about FineIBT *at*all*. Additionally, one can inhibit patching of FineIBT by booting with 'cfi=kcfi' on IBT enabled hardware. And in that case (kCFI+IBT), we'll do the caller hash check and still jump to +0, so there really must be an ENDBR there. Only if we were to dis-allow this combination could we say the ENDBR at +0 becomes superfluous and should find means for the compiler not emit it. > > Having this second ENDBR however makes it possible to elide the CFI > > check. Therefore, we should poison this second ENDBR (if present) when > > switching to FineIBT mode. > > > > Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT") > > Reported-by: "Milburn, Alyssa" <alyssa.milburn@intel.com> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > Looks like a good work-around. > > Acked-by: Kees Cook <keescook@chromium.org> Thanks!
On Wed, Jun 21, 2023 at 10:18:57AM +0200, Peter Zijlstra wrote: > (and I need to write a better Changelog). Updated changelog... --- Subject: x86/fineibt: Poison ENDBR at +0 From: Peter Zijlstra <peterz@infradead.org> Date: Thu, 15 Jun 2023 21:35:48 +0200 Alyssa noticed that when building the kernel with CFI_CLANG+IBT and booting on IBT enabled hardware obtain FineIBT, the indirect functions look like: __cfi_foo: endbr64 subl $hash, %r10d jz 1f ud2 nop 1: foo: endbr64 This is because the compiler generates code for kCFI+IBT. In that case the caller does the hash check and will jump to +0, so there must be an ENDBR there. The compiler doesn't know about FineIBT at all; also it is possible to actually use kCFI+IBT when booting with 'cfi=kcfi' on IBT enabled hardware. Having this second ENDBR however makes it possible to elide the CFI check. Therefore, we should poison this second ENDBR when switching to FineIBT mode. Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT") Reported-by: "Milburn, Alyssa" <alyssa.milburn@intel.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Acked-by: Kees Cook <keescook@chromium.org> Link: https://lore.kernel.org/r/20230615193722.194131053@infradead.org --- arch/x86/kernel/alternative.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -1063,6 +1063,17 @@ static int cfi_rewrite_preamble(s32 *sta return 0; } +static void cfi_rewrite_endbr(s32 *start, s32 *end) +{ + s32 *s; + + for (s = start; s < end; s++) { + void *addr = (void *)s + *s; + + poison_endbr(addr+16, false); + } +} + /* .retpoline_sites */ static int cfi_rand_callers(s32 *start, s32 *end) { @@ -1157,14 +1168,19 @@ static void __apply_fineibt(s32 *start_r return; case CFI_FINEIBT: + /* place the FineIBT preamble at func()-16 */ ret = cfi_rewrite_preamble(start_cfi, end_cfi); if (ret) goto err; + /* rewrite the callers to target func()-16 */ ret = cfi_rewrite_callers(start_retpoline, end_retpoline); if (ret) goto err; + /* now that nobody targets func()+0, remove ENDBR there */ + cfi_rewrite_endbr(start_cfi, end_cfi); + if (builtin) pr_info("Using FineIBT CFI\n"); return;
On Tue, Jun 20, 2023 at 05:04:50PM -0700, Sami Tolvanen wrote: > On Tue, Jun 20, 2023 at 2:55 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote: > > > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and > > > booting on IBT enabled hardware obtain FineIBT, the indirect functions > > > look like: > > > > > > __cfi_foo: > > > endbr64 > > > subl $hash, %r10d > > > jz 1f > > > ud2 > > > nop > > > 1: > > > foo: > > > endbr64 > > > > > > This is because clang currently does not supress ENDBR emission for > > > functions it provides a __cfi prologue symbol for. > > > > Should this be considered a bug in Clang? > > The endbr is needed with KCFI if we have FineIBT disabled for some > reason. There's some discussion here: > > https://github.com/ClangBuiltLinux/linux/issues/1735 > > However, since the kernel handles FineIBT patching, it might be > cleaner to let it also poison the extra endbr. That's what I get for replying before reading all replies. Anyway, we're in agreement.
On Wed, Jun 21, 2023 at 10:48:02AM +0200, Peter Zijlstra wrote: > On Wed, Jun 21, 2023 at 10:18:57AM +0200, Peter Zijlstra wrote: > > (and I need to write a better Changelog). > > Updated changelog... LGTM! Thanks :)
--- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -940,6 +940,17 @@ static int cfi_rewrite_preamble(s32 *sta return 0; } +static void cfi_rewrite_endbr(s32 *start, s32 *end) +{ + s32 *s; + + for (s = start; s < end; s++) { + void *addr = (void *)s + *s; + + poison_endbr(addr+16, false); + } +} + /* .retpoline_sites */ static int cfi_rand_callers(s32 *start, s32 *end) { @@ -1034,14 +1045,19 @@ static void __apply_fineibt(s32 *start_r return; case CFI_FINEIBT: + /* place the FineIBT preamble at func()-16 */ ret = cfi_rewrite_preamble(start_cfi, end_cfi); if (ret) goto err; + /* rewrite the callers to target func()-16 */ ret = cfi_rewrite_callers(start_retpoline, end_retpoline); if (ret) goto err; + /* now that nobody targets func()+0, remove ENDBR there */ + cfi_rewrite_endbr(start_cfi, end_cfi); + if (builtin) pr_info("Using FineIBT CFI\n"); return;