Messages in this thread | | | Date | Mon, 4 Apr 2022 19:53:05 -0700 | From | Kees Cook <> | Subject | Re: [PATCH 1/3] kbuild: Change CFI_CLANG to depend on __builtin_function_start |
| |
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
| |