Re: [PATCH 1/3] kbuild: Change CFI_CLANG to depend on __builtin_function_start

From: Kees Cook <keescook@chromium.org>
To: Nathan Chancellor <nathan@kernel.org>
Cc: Sami Tolvanen <samitolvanen@google.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	llvm@lists.linux.dev
Subject: Re: [PATCH 1/3] kbuild: Change CFI_CLANG to depend on __builtin_function_start
Date: Mon, 4 Apr 2022 19:53:05 -0700	[thread overview]
Message-ID: <202204041950.B13AD5CB@keescook> (raw)
In-Reply-To: <Ykt2mz3gBTAyu9pL@dev-arch.thelio-3990X>

On Mon, Apr 04, 2022 at 03:52:11PM -0700, Nathan Chancellor wrote:
> On Mon, Apr 04, 2022 at 12:40:46PM -0700, Sami Tolvanen wrote:
> > On Sat, Apr 2, 2022 at 6:32 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > On Sat, Apr 2, 2022 at 5:19 AM Sami Tolvanen <samitolvanen@google.com> wrote:
> > > >
> > > > Clang 14 added support for the __builtin_function_start()
> > > > built-in function, which allows us to implement function_nocfi()
> > > > without architecture-specific inline assembly. This patch changes
> > > > CONFIG_CFI_CLANG to depend on the built-in and effectively upgrades
> > > > the minimum supported compiler version for CFI to Clang 14.
> > >
> > > From this description, I think the straight-forward change would be:
> > >
> > >     depends on CLANG_VERSION >= 120000
> > > -->
> > >     depends on CLANG_VERSION >= 140000
> > >
> > > Any reason to avoid this?
> > 
> > I thought testing for the compiler feature was preferred, but I can
> > certainly just increase the minimum version number here too.
> 
> I think we have been somewhat inconsistent with feature versus version
> checking. It might be nice to hash out when a feature check should be
> done instead of a version one.
> 
> Generally, I think we tend to prefer version checks, as they are
> "cheaper" since we do not have to call the compiler again because we
> already cached the version code. When adding version checks, our policy
> has always been use the upstream version of LLVM that the feature in
> question shipped in, even if it is a top of tree version, as people who
> are using prereleased versions of LLVM should be frequently updating
> them.
> 
> Unfortunately, that does not always match reality. For example,
> Android's LLVM tracks the main branch but they are almost always behind
> by a few months. For example, the latest release is 14.0.4, based on a
> version of LLVM from January 28th:
> 
> https://android.googlesource.com/platform/prebuilts/clang/host/linux-x86/+/ab73cd180863dbd17fdb8f20e39b33ab38030cf9/clang-r450784b/clang_source_info.md
> https://github.com/llvm/llvm-project/commits/282c83c32384cb2f37030c28650fef4150a8b67c
> 
> Normally, I would say "who cares?" but Android's LLVM is used by the
> Android kernel team both downstream and upstream, so I would argue it is
> important to take that into account when deciding to do a feature check
> versus a version check. In other words, by moving to a version check,
> will we knowingly break a version of clang that is relatively widely
> used?
> 
> In this case, 14.0.4 has __builtin_function_start(), so I think it is
> okay to use a version check instead of a feature one.

Thanks for checking the details on that. Yeah, I think it's fine to go
with a version check here.

Sami, can you send a v2, and I can take it via the hardening for
-next? (Unless the ARM folks _really_ want it for -rc2 -- this is kind
of a fix, but it's also kind of not.)

-- 
Kees Cook