linux-kernel - Re: [PATCH 1/3] kbuild: Change CFI_CLANG to depend on __builtin_function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202204041950.B13AD5CB@keescook>
Date:   Mon, 4 Apr 2022 19:53:05 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Nathan Chancellor <nathan@...nel.org>
Cc:     Sami Tolvanen <samitolvanen@...gle.com>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Mark Rutland <mark.rutland@....com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Will Deacon <will@...nel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        llvm@...ts.linux.dev
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@...nel.org> wrote:
> > >
> > > On Sat, Apr 2, 2022 at 5:19 AM Sami Tolvanen <samitolvanen@...gle.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