[PATCH 0/3] cfi: Use __builtin_function_start

[PATCH 0/3] cfi: Use __builtin_function_start

[Sami Tolvanen]

Clang 14 introduced the __builtin_function_start intrinsic, which
makes it possible to cleanly implement the function_nocfi() macro in a
way that also works with static initializers. Add a default
function_nocfi() implementation and drop the arm64 inline assembly
version.

Sami Tolvanen (3):
  kbuild: Change CFI_CLANG to depend on __builtin_function_start
  linux/compiler-clang.h: define function_nocfi
  arm64: Drop the inline assembly implementation of function_nocfi

 arch/Kconfig                      |  5 +----
 arch/arm64/include/asm/compiler.h | 16 ----------------
 include/linux/compiler-clang.h    | 10 ++++++++++
 init/Kconfig                      |  3 +++
 4 files changed, 14 insertions(+), 20 deletions(-)

-- 
2.35.0

[PATCH 0/3] cfi: Use __builtin_function_start

[Sami Tolvanen]

Clang 14 introduced the __builtin_function_start intrinsic, which
makes it possible to cleanly implement the function_nocfi() macro in a
way that also works with static initializers. Add a default
function_nocfi() implementation and drop the arm64 inline assembly
version.

Sami Tolvanen (3):
  kbuild: Change CFI_CLANG to depend on __builtin_function_start
  linux/compiler-clang.h: define function_nocfi
  arm64: Drop the inline assembly implementation of function_nocfi

 arch/Kconfig                      |  5 +----
 arch/arm64/include/asm/compiler.h | 16 ----------------
 include/linux/compiler-clang.h    | 10 ++++++++++
 init/Kconfig                      |  3 +++
 4 files changed, 14 insertions(+), 20 deletions(-)

-- 
2.35.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sami Tolvanen]

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.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/Kconfig | 5 +----
 init/Kconfig | 3 +++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 29b0167c088b..eecfc2809781 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -723,10 +723,7 @@ config ARCH_SUPPORTS_CFI_CLANG
 config CFI_CLANG
 	bool "Use Clang's Control Flow Integrity (CFI)"
 	depends on LTO_CLANG && ARCH_SUPPORTS_CFI_CLANG
-	# Clang >= 12:
-	# - https://bugs.llvm.org/show_bug.cgi?id=46258
-	# - https://bugs.llvm.org/show_bug.cgi?id=47479
-	depends on CLANG_VERSION >= 120000
+	depends on CC_HAS_BUILTIN_FUNCTION_START
 	select KALLSYMS
 	help
 	  This option enables Clang’s forward-edge Control Flow Integrity
diff --git a/init/Kconfig b/init/Kconfig
index ddcbefe535e9..f024fd353373 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -86,6 +86,9 @@ config CC_HAS_ASM_INLINE
 config CC_HAS_NO_PROFILE_FN_ATTR
 	def_bool $(success,echo '__attribute__((no_profile_instrument_function)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
 
+config CC_HAS_BUILTIN_FUNCTION_START
+	def_bool $(success,echo 'void f(void) {}; void *p = __builtin_function_start(f);' | $(CC) -x c - -c -o /dev/null)
+
 config PAHOLE_VERSION
 	int
 	default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE))
-- 
2.35.0

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

[Sami Tolvanen]

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.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/Kconfig | 5 +----
 init/Kconfig | 3 +++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 29b0167c088b..eecfc2809781 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -723,10 +723,7 @@ config ARCH_SUPPORTS_CFI_CLANG
 config CFI_CLANG
 	bool "Use Clang's Control Flow Integrity (CFI)"
 	depends on LTO_CLANG && ARCH_SUPPORTS_CFI_CLANG
-	# Clang >= 12:
-	# - https://bugs.llvm.org/show_bug.cgi?id=46258
-	# - https://bugs.llvm.org/show_bug.cgi?id=47479
-	depends on CLANG_VERSION >= 120000
+	depends on CC_HAS_BUILTIN_FUNCTION_START
 	select KALLSYMS
 	help
 	  This option enables Clang’s forward-edge Control Flow Integrity
diff --git a/init/Kconfig b/init/Kconfig
index ddcbefe535e9..f024fd353373 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -86,6 +86,9 @@ config CC_HAS_ASM_INLINE
 config CC_HAS_NO_PROFILE_FN_ATTR
 	def_bool $(success,echo '__attribute__((no_profile_instrument_function)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
 
+config CC_HAS_BUILTIN_FUNCTION_START
+	def_bool $(success,echo 'void f(void) {}; void *p = __builtin_function_start(f);' | $(CC) -x c - -c -o /dev/null)
+
 config PAHOLE_VERSION
 	int
 	default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE))
-- 
2.35.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Masahiro Yamada]

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?


>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/Kconfig | 5 +----
>  init/Kconfig | 3 +++
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 29b0167c088b..eecfc2809781 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -723,10 +723,7 @@ config ARCH_SUPPORTS_CFI_CLANG
>  config CFI_CLANG
>         bool "Use Clang's Control Flow Integrity (CFI)"
>         depends on LTO_CLANG && ARCH_SUPPORTS_CFI_CLANG
> -       # Clang >= 12:
> -       # - https://bugs.llvm.org/show_bug.cgi?id=46258
> -       # - https://bugs.llvm.org/show_bug.cgi?id=47479
> -       depends on CLANG_VERSION >= 120000
> +       depends on CC_HAS_BUILTIN_FUNCTION_START
>         select KALLSYMS
>         help
>           This option enables Clang’s forward-edge Control Flow Integrity
> diff --git a/init/Kconfig b/init/Kconfig
> index ddcbefe535e9..f024fd353373 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -86,6 +86,9 @@ config CC_HAS_ASM_INLINE
>  config CC_HAS_NO_PROFILE_FN_ATTR
>         def_bool $(success,echo '__attribute__((no_profile_instrument_function)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
>
> +config CC_HAS_BUILTIN_FUNCTION_START
> +       def_bool $(success,echo 'void f(void) {}; void *p = __builtin_function_start(f);' | $(CC) -x c - -c -o /dev/null)
> +
>  config PAHOLE_VERSION
>         int
>         default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE))
> --
> 2.35.0
>


--
Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

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?


>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/Kconfig | 5 +----
>  init/Kconfig | 3 +++
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 29b0167c088b..eecfc2809781 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -723,10 +723,7 @@ config ARCH_SUPPORTS_CFI_CLANG
>  config CFI_CLANG
>         bool "Use Clang's Control Flow Integrity (CFI)"
>         depends on LTO_CLANG && ARCH_SUPPORTS_CFI_CLANG
> -       # Clang >= 12:
> -       # - https://bugs.llvm.org/show_bug.cgi?id=46258
> -       # - https://bugs.llvm.org/show_bug.cgi?id=47479
> -       depends on CLANG_VERSION >= 120000
> +       depends on CC_HAS_BUILTIN_FUNCTION_START
>         select KALLSYMS
>         help
>           This option enables Clang’s forward-edge Control Flow Integrity
> diff --git a/init/Kconfig b/init/Kconfig
> index ddcbefe535e9..f024fd353373 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -86,6 +86,9 @@ config CC_HAS_ASM_INLINE
>  config CC_HAS_NO_PROFILE_FN_ATTR
>         def_bool $(success,echo '__attribute__((no_profile_instrument_function)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
>
> +config CC_HAS_BUILTIN_FUNCTION_START
> +       def_bool $(success,echo 'void f(void) {}; void *p = __builtin_function_start(f);' | $(CC) -x c - -c -o /dev/null)
> +
>  config PAHOLE_VERSION
>         int
>         default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE))
> --
> 2.35.0
>


--
Best Regards
Masahiro Yamada

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sami Tolvanen]

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.

Sami

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

[Sami Tolvanen]

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.

Sami

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nathan Chancellor]

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.

There are times where feature or problem checking is not always
possible:

https://lore.kernel.org/r/20220318230747.3900772-1-nathan@kernel.org/

In cases like these, we can work to upgrade the compiler before changing
the feature check ot a version one, which helps minimize the window for
breakage.

Another aspect of feature versus version checks is that it is easier to
clean up stale versions checks when we bump the minimum supported
version of the compiler (as we can just look for "CLANG_VERSION" across
the tree) compared to stale feature checks. We could fix this by adding
some sort of common keyword, like

Compiler check: <compiler> <comparison> <version>

like

Compiler check: clang <= 14.0.0

Cheers,
Nathan

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

[Nathan Chancellor]

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.

There are times where feature or problem checking is not always
possible:

https://lore.kernel.org/r/20220318230747.3900772-1-nathan@kernel.org/

In cases like these, we can work to upgrade the compiler before changing
the feature check ot a version one, which helps minimize the window for
breakage.

Another aspect of feature versus version checks is that it is easier to
clean up stale versions checks when we bump the minimum supported
version of the compiler (as we can just look for "CLANG_VERSION" across
the tree) compared to stale feature checks. We could fix this by adding
some sort of common keyword, like

Compiler check: <compiler> <comparison> <version>

like

Compiler check: clang <= 14.0.0

Cheers,
Nathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Kees Cook]

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

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

[Kees Cook]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] linux/compiler-clang.h: define function_nocfi

[Sami Tolvanen]

Use __builtin_function_start() to implement the function_nocfi() macro
when CONFIG_CFI_CLANG is selected.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 include/linux/compiler-clang.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index babb1347148c..c84fec767445 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -69,6 +69,16 @@
 #define __nocfi		__attribute__((__no_sanitize__("cfi")))
 #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
 
+#if defined(CONFIG_CFI_CLANG)
+/*
+ * With CONFIG_CFI_CLANG, the compiler replaces function address
+ * references with the address of the function's CFI jump table
+ * entry. The function_nocfi macro always returns the address of the
+ * actual function instead.
+ */
+#define function_nocfi(x)	__builtin_function_start(x)
+#endif
+
 /*
  * Turn individual warnings and errors on and off locally, depending
  * on version.
-- 
2.35.0

[PATCH 2/3] linux/compiler-clang.h: define function_nocfi

[Sami Tolvanen]

Use __builtin_function_start() to implement the function_nocfi() macro
when CONFIG_CFI_CLANG is selected.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 include/linux/compiler-clang.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index babb1347148c..c84fec767445 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -69,6 +69,16 @@
 #define __nocfi		__attribute__((__no_sanitize__("cfi")))
 #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
 
+#if defined(CONFIG_CFI_CLANG)
+/*
+ * With CONFIG_CFI_CLANG, the compiler replaces function address
+ * references with the address of the function's CFI jump table
+ * entry. The function_nocfi macro always returns the address of the
+ * actual function instead.
+ */
+#define function_nocfi(x)	__builtin_function_start(x)
+#endif
+
 /*
  * Turn individual warnings and errors on and off locally, depending
  * on version.
-- 
2.35.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] linux/compiler-clang.h: define function_nocfi

[Mark Rutland]

On Fri, Apr 01, 2022 at 01:19:15PM -0700, Sami Tolvanen wrote:
> Use __builtin_function_start() to implement the function_nocfi() macro
> when CONFIG_CFI_CLANG is selected.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  include/linux/compiler-clang.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index babb1347148c..c84fec767445 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -69,6 +69,16 @@
>  #define __nocfi		__attribute__((__no_sanitize__("cfi")))
>  #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
>  
> +#if defined(CONFIG_CFI_CLANG)
> +/*
> + * With CONFIG_CFI_CLANG, the compiler replaces function address
> + * references with the address of the function's CFI jump table
> + * entry. The function_nocfi macro always returns the address of the
> + * actual function instead.
> + */
> +#define function_nocfi(x)	__builtin_function_start(x)
> +#endif

As a heads-up, with patches 1 and 2 applied (but not patch 3), we have two
definitions of function_nocfi(), which casues the compiler to scream for every
object:

| In file included from <built-in>:3:
| In file included from ././include/linux/compiler_types.h:107:
| ./arch/arm64/include/asm/compiler.h:33:9: warning: 'function_nocfi' macro redefined [-Wmacro-redefined]
| #define function_nocfi(x) ({                                            \
|         ^
| ./include/linux/compiler-clang.h:79:9: note: previous definition is here
| #define function_nocfi(x)       __builtin_function_start(x)
|         ^
| 1 warning generated.

... which would be painful for bisection.

I suspect It's probably worth folding patches 2 and 3 together to avoid that?

Arguably we could fold the entire series together into one commit at that
point.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] linux/compiler-clang.h: define function_nocfi

[Mark Rutland]

On Fri, Apr 01, 2022 at 01:19:15PM -0700, Sami Tolvanen wrote:
> Use __builtin_function_start() to implement the function_nocfi() macro
> when CONFIG_CFI_CLANG is selected.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  include/linux/compiler-clang.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index babb1347148c..c84fec767445 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -69,6 +69,16 @@
>  #define __nocfi		__attribute__((__no_sanitize__("cfi")))
>  #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
>  
> +#if defined(CONFIG_CFI_CLANG)
> +/*
> + * With CONFIG_CFI_CLANG, the compiler replaces function address
> + * references with the address of the function's CFI jump table
> + * entry. The function_nocfi macro always returns the address of the
> + * actual function instead.
> + */
> +#define function_nocfi(x)	__builtin_function_start(x)
> +#endif

As a heads-up, with patches 1 and 2 applied (but not patch 3), we have two
definitions of function_nocfi(), which casues the compiler to scream for every
object:

| In file included from <built-in>:3:
| In file included from ././include/linux/compiler_types.h:107:
| ./arch/arm64/include/asm/compiler.h:33:9: warning: 'function_nocfi' macro redefined [-Wmacro-redefined]
| #define function_nocfi(x) ({                                            \
|         ^
| ./include/linux/compiler-clang.h:79:9: note: previous definition is here
| #define function_nocfi(x)       __builtin_function_start(x)
|         ^
| 1 warning generated.

... which would be painful for bisection.

I suspect It's probably worth folding patches 2 and 3 together to avoid that?

Arguably we could fold the entire series together into one commit at that
point.

Thanks,
Mark.

[PATCH 3/3] arm64: Drop the inline assembly implementation of function_nocfi

[Sami Tolvanen]

Drop the inline assembly version of function_nocfi() now that
CONFIG_CFI_CLANG depends on __builtin_function_start(), which allows
this macro to be implemented more cleanly and in a way that works also
with static initializers.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/arm64/include/asm/compiler.h | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
index dc3ea4080e2e..6fb2e6bcc392 100644
--- a/arch/arm64/include/asm/compiler.h
+++ b/arch/arm64/include/asm/compiler.h
@@ -23,20 +23,4 @@
 #define __builtin_return_address(val)					\
 	(void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val)))
 
-#ifdef CONFIG_CFI_CLANG
-/*
- * With CONFIG_CFI_CLANG, the compiler replaces function address
- * references with the address of the function's CFI jump table
- * entry. The function_nocfi macro always returns the address of the
- * actual function instead.
- */
-#define function_nocfi(x) ({						\
-	void *addr;							\
-	asm("adrp %0, " __stringify(x) "\n\t"				\
-	    "add  %0, %0, :lo12:" __stringify(x)			\
-	    : "=r" (addr));						\
-	addr;								\
-})
-#endif
-
 #endif /* __ASM_COMPILER_H */
-- 
2.35.0

[PATCH 3/3] arm64: Drop the inline assembly implementation of function_nocfi

[Sami Tolvanen]

Drop the inline assembly version of function_nocfi() now that
CONFIG_CFI_CLANG depends on __builtin_function_start(), which allows
this macro to be implemented more cleanly and in a way that works also
with static initializers.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/arm64/include/asm/compiler.h | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
index dc3ea4080e2e..6fb2e6bcc392 100644
--- a/arch/arm64/include/asm/compiler.h
+++ b/arch/arm64/include/asm/compiler.h
@@ -23,20 +23,4 @@
 #define __builtin_return_address(val)					\
 	(void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val)))
 
-#ifdef CONFIG_CFI_CLANG
-/*
- * With CONFIG_CFI_CLANG, the compiler replaces function address
- * references with the address of the function's CFI jump table
- * entry. The function_nocfi macro always returns the address of the
- * actual function instead.
- */
-#define function_nocfi(x) ({						\
-	void *addr;							\
-	asm("adrp %0, " __stringify(x) "\n\t"				\
-	    "add  %0, %0, :lo12:" __stringify(x)			\
-	    : "=r" (addr));						\
-	addr;								\
-})
-#endif
-
 #endif /* __ASM_COMPILER_H */
-- 
2.35.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] arm64: Drop the inline assembly implementation of function_nocfi

[Will Deacon]

On Fri, Apr 01, 2022 at 01:19:16PM -0700, Sami Tolvanen wrote:
> Drop the inline assembly version of function_nocfi() now that
> CONFIG_CFI_CLANG depends on __builtin_function_start(), which allows
> this macro to be implemented more cleanly and in a way that works also
> with static initializers.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/arm64/include/asm/compiler.h | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> index dc3ea4080e2e..6fb2e6bcc392 100644
> --- a/arch/arm64/include/asm/compiler.h
> +++ b/arch/arm64/include/asm/compiler.h
> @@ -23,20 +23,4 @@
>  #define __builtin_return_address(val)					\
>  	(void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val)))
>  
> -#ifdef CONFIG_CFI_CLANG
> -/*
> - * With CONFIG_CFI_CLANG, the compiler replaces function address
> - * references with the address of the function's CFI jump table
> - * entry. The function_nocfi macro always returns the address of the
> - * actual function instead.
> - */
> -#define function_nocfi(x) ({						\
> -	void *addr;							\
> -	asm("adrp %0, " __stringify(x) "\n\t"				\
> -	    "add  %0, %0, :lo12:" __stringify(x)			\
> -	    : "=r" (addr));						\
> -	addr;								\
> -})
> -#endif
> -
>  #endif /* __ASM_COMPILER_H */

Acked-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH 3/3] arm64: Drop the inline assembly implementation of function_nocfi

[Will Deacon]

On Fri, Apr 01, 2022 at 01:19:16PM -0700, Sami Tolvanen wrote:
> Drop the inline assembly version of function_nocfi() now that
> CONFIG_CFI_CLANG depends on __builtin_function_start(), which allows
> this macro to be implemented more cleanly and in a way that works also
> with static initializers.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/arm64/include/asm/compiler.h | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> index dc3ea4080e2e..6fb2e6bcc392 100644
> --- a/arch/arm64/include/asm/compiler.h
> +++ b/arch/arm64/include/asm/compiler.h
> @@ -23,20 +23,4 @@
>  #define __builtin_return_address(val)					\
>  	(void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val)))
>  
> -#ifdef CONFIG_CFI_CLANG
> -/*
> - * With CONFIG_CFI_CLANG, the compiler replaces function address
> - * references with the address of the function's CFI jump table
> - * entry. The function_nocfi macro always returns the address of the
> - * actual function instead.
> - */
> -#define function_nocfi(x) ({						\
> -	void *addr;							\
> -	asm("adrp %0, " __stringify(x) "\n\t"				\
> -	    "add  %0, %0, :lo12:" __stringify(x)			\
> -	    : "=r" (addr));						\
> -	addr;								\
> -})
> -#endif
> -
>  #endif /* __ASM_COMPILER_H */

Acked-by: Will Deacon <will@kernel.org>

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] arm64: Drop the inline assembly implementation of function_nocfi

[Mark Rutland]

On Fri, Apr 01, 2022 at 01:19:16PM -0700, Sami Tolvanen wrote:
> Drop the inline assembly version of function_nocfi() now that
> CONFIG_CFI_CLANG depends on __builtin_function_start(), which allows
> this macro to be implemented more cleanly and in a way that works also
> with static initializers.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/arm64/include/asm/compiler.h | 16 ----------------
>  1 file changed, 16 deletions(-)

It's very nice to see the asm go here!

I grabbed the LLVM 14.0.0 binary release from llvm.org and gave this a spin. I
tested with ThinLTO and CFI_CLANG, and verified that CPU hotplug and ftrace
worked as expected (given those rely on function_nocfi()), and that the LKDTM
CFI_FORWARD_PROTO caught the mismatched prototype as expected.

FWIW:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

As noted on patch 2 I think we should squash the series together, but those
tags apply either way.

Thanks,
Mark.

> 
> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> index dc3ea4080e2e..6fb2e6bcc392 100644
> --- a/arch/arm64/include/asm/compiler.h
> +++ b/arch/arm64/include/asm/compiler.h
> @@ -23,20 +23,4 @@
>  #define __builtin_return_address(val)					\
>  	(void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val)))
>  
> -#ifdef CONFIG_CFI_CLANG
> -/*
> - * With CONFIG_CFI_CLANG, the compiler replaces function address
> - * references with the address of the function's CFI jump table
> - * entry. The function_nocfi macro always returns the address of the
> - * actual function instead.
> - */
> -#define function_nocfi(x) ({						\
> -	void *addr;							\
> -	asm("adrp %0, " __stringify(x) "\n\t"				\
> -	    "add  %0, %0, :lo12:" __stringify(x)			\
> -	    : "=r" (addr));						\
> -	addr;								\
> -})
> -#endif
> -
>  #endif /* __ASM_COMPILER_H */
> -- 
> 2.35.0
> 

Re: [PATCH 3/3] arm64: Drop the inline assembly implementation of function_nocfi

[Mark Rutland]

On Fri, Apr 01, 2022 at 01:19:16PM -0700, Sami Tolvanen wrote:
> Drop the inline assembly version of function_nocfi() now that
> CONFIG_CFI_CLANG depends on __builtin_function_start(), which allows
> this macro to be implemented more cleanly and in a way that works also
> with static initializers.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/arm64/include/asm/compiler.h | 16 ----------------
>  1 file changed, 16 deletions(-)

It's very nice to see the asm go here!

I grabbed the LLVM 14.0.0 binary release from llvm.org and gave this a spin. I
tested with ThinLTO and CFI_CLANG, and verified that CPU hotplug and ftrace
worked as expected (given those rely on function_nocfi()), and that the LKDTM
CFI_FORWARD_PROTO caught the mismatched prototype as expected.

FWIW:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

As noted on patch 2 I think we should squash the series together, but those
tags apply either way.

Thanks,
Mark.

> 
> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> index dc3ea4080e2e..6fb2e6bcc392 100644
> --- a/arch/arm64/include/asm/compiler.h
> +++ b/arch/arm64/include/asm/compiler.h
> @@ -23,20 +23,4 @@
>  #define __builtin_return_address(val)					\
>  	(void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val)))
>  
> -#ifdef CONFIG_CFI_CLANG
> -/*
> - * With CONFIG_CFI_CLANG, the compiler replaces function address
> - * references with the address of the function's CFI jump table
> - * entry. The function_nocfi macro always returns the address of the
> - * actual function instead.
> - */
> -#define function_nocfi(x) ({						\
> -	void *addr;							\
> -	asm("adrp %0, " __stringify(x) "\n\t"				\
> -	    "add  %0, %0, :lo12:" __stringify(x)			\
> -	    : "=r" (addr));						\
> -	addr;								\
> -})
> -#endif
> -
>  #endif /* __ASM_COMPILER_H */
> -- 
> 2.35.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] cfi: Use __builtin_function_start

[Nick Desaulniers]

On Fri, Apr 1, 2022 at 1:19 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> Clang 14 introduced the __builtin_function_start intrinsic, which
> makes it possible to cleanly implement the function_nocfi() macro in a
> way that also works with static initializers. Add a default
> function_nocfi() implementation and drop the arm64 inline assembly
> version.

For the series
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

I wouldn't mind if patch 1/3 included a link to the llvm commit in the
commit message.

Link: https://github.com/llvm/llvm-project/commit/ec2e26eaf63558934f5b73a6e530edc453cf9508
Link: https://github.com/ClangBuiltLinux/linux/issues/1353

>
> Sami Tolvanen (3):
>   kbuild: Change CFI_CLANG to depend on __builtin_function_start
>   linux/compiler-clang.h: define function_nocfi
>   arm64: Drop the inline assembly implementation of function_nocfi
>
>  arch/Kconfig                      |  5 +----
>  arch/arm64/include/asm/compiler.h | 16 ----------------
>  include/linux/compiler-clang.h    | 10 ++++++++++
>  init/Kconfig                      |  3 +++
>  4 files changed, 14 insertions(+), 20 deletions(-)
>
> --
> 2.35.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 0/3] cfi: Use __builtin_function_start

[Nick Desaulniers]

On Fri, Apr 1, 2022 at 1:19 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> Clang 14 introduced the __builtin_function_start intrinsic, which
> makes it possible to cleanly implement the function_nocfi() macro in a
> way that also works with static initializers. Add a default
> function_nocfi() implementation and drop the arm64 inline assembly
> version.

For the series
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

I wouldn't mind if patch 1/3 included a link to the llvm commit in the
commit message.

Link: https://github.com/llvm/llvm-project/commit/ec2e26eaf63558934f5b73a6e530edc453cf9508
Link: https://github.com/ClangBuiltLinux/linux/issues/1353

>
> Sami Tolvanen (3):
>   kbuild: Change CFI_CLANG to depend on __builtin_function_start
>   linux/compiler-clang.h: define function_nocfi
>   arm64: Drop the inline assembly implementation of function_nocfi
>
>  arch/Kconfig                      |  5 +----
>  arch/arm64/include/asm/compiler.h | 16 ----------------
>  include/linux/compiler-clang.h    | 10 ++++++++++
>  init/Kconfig                      |  3 +++
>  4 files changed, 14 insertions(+), 20 deletions(-)
>
> --
> 2.35.0
>


-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel