There is a bug in the LLVM implementation of KASAN and GCOV that makes
these options incompatible with the CFI_ICALL_NORMALIZE_INTEGERS option.

The bug has already been fixed in llvm/clang [1] and rustc [2]. However,
Kconfig currently has no way to gate features on the LLVM version inside
rustc, so we cannot write down a precise `depends on` clause in this
case. Instead, a `def_bool` option is defined for whether
CFI_ICALL_NORMALIZE_INTEGERS is available, and its default value is set
to false when GCOV or KASAN are turned on. End users using a patched
clang/rustc can turn on the HAVE_CFI_ICALL_NORMALIZE_INTEGERS option
directly to override this.

An alternative solution is to inspect a binary created by clang or rustc
to see whether the faulty CFI tags are in the binary. This would be a
precise check, but it would involve hard-coding the *hashed* version of
the CFI tag. This is because there's no way to get clang or rustc to
output the unhased version of the CFI tag. Relying on the precise
hashing algorithm using by CFI seems too fragile, so I have not pursued
this option. Besides, this kind of hack is exactly what lead to the LLVM
bug in the first place.

If the CFI_ICALL_NORMALIZE_INTEGERS option is used without CONFIG_RUST,
then we actually can perform a precise check today: just compare the
clang version number. This works since clang and llvm are always updated
in lockstep. However, encoding this in Kconfig would give the
HAVE_CFI_ICALL_NORMALIZE_INTEGERS option a dependency on CONFIG_RUST,
which is not possible as the reverse dependency already exists.

HAVE_CFI_ICALL_NORMALIZE_INTEGERS is defined to be a `def_bool` instead
of `bool` to avoid asking end users whether they want to turn on the
option. Turning it on explicitly is something only experts should do, so
making it hard to do so is not an issue.

I added a `depends on CFI_CLANG` clause to the new Kconfig option. I'm
not sure whether that makes sense or not, but it doesn't seem to make a
big difference.

In a future kernel release, I would like to add a Kconfig option similar
to CLANG_VERSION/RUSTC_VERSION for inspecting the version of the LLVM
inside rustc. Once that feature lands, this logic will be replaced with
a precise version check. This check is not being introduced here to
avoid introducing a new _VERSION constant in a fix.

Link: https://github.com/llvm/llvm-project/pull/104826 [1]
Link: https://github.com/rust-lang/rust/pull/129373 [2]
Fixes: ce4a2620985c ("cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS")
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202409231044.4f064459-oliver.sang@intel.com
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 arch/Kconfig | 18 +++++++++++++++++-
 init/Kconfig |  2 +-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index ee58df8b1080..b8066bf43153 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -829,7 +829,7 @@ config CFI_CLANG
 config CFI_ICALL_NORMALIZE_INTEGERS
 	bool "Normalize CFI tags for integers"
 	depends on CFI_CLANG
-	depends on $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)
+	depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS
 	help
 	  This option normalizes the CFI tags for integer types so that all
 	  integer types of the same size and signedness receive the same CFI
@@ -842,6 +842,22 @@ config CFI_ICALL_NORMALIZE_INTEGERS
 
 	  This option is necessary for using CFI with Rust. If unsure, say N.
 
+config HAVE_CFI_ICALL_NORMALIZE_INTEGERS
+	def_bool !GCOV_KERNEL && !KASAN
+	depends on CFI_CLANG
+	depends on $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)
+	help
+	  Is CFI_ICALL_NORMALIZE_INTEGERS supported with the set of compilers
+	  currently in use?
+
+	  This option defaults to false if GCOV or KASAN is enabled, as there is
+	  an LLVM bug that makes normalized integers tags incompatible with
+	  KASAN and GCOV. Kconfig currently does not have the infrastructure to
+	  detect whether your rustc compiler contains the fix for this bug, so
+	  it is assumed that it doesn't. If your compiler has the fix, you can
+	  explicitly enable this option in your config file. The Kconfig logic
+	  needed to detect this will be added in a future kernel release.
+
 config CFI_PERMISSIVE
 	bool "Use CFI in permissive mode"
 	depends on CFI_CLANG
diff --git a/init/Kconfig b/init/Kconfig
index 4ea2a161d362..6f3f36d631f6 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1909,7 +1909,7 @@ config RUST
 	depends on !GCC_PLUGIN_RANDSTRUCT
 	depends on !RANDSTRUCT
 	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
-	depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)
+	depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && HAVE_CFI_ICALL_NORMALIZE_INTEGERS
 	select CFI_ICALL_NORMALIZE_INTEGERS if CFI_CLANG
 	depends on !CALL_PADDING || RUSTC_VERSION >= 108000
 	depends on !KASAN_SW_TAGS

---
base-commit: a2f11547052001bd448ccec81dd1e68409078fbb
change-id: 20240924-cfi-norm-kasan-fix-71332665cc5c

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

On Wed, Sep 25, 2024 at 10:10 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> There is a bug in the LLVM implementation of KASAN and GCOV that makes
> these options incompatible with the CFI_ICALL_NORMALIZE_INTEGERS option.

Applied to `rust-fixes` -- thanks everyone!

Cheers,
Miguel

Hi Alice,

On Wed, Sep 25, 2024 at 08:10:18AM +0000, Alice Ryhl wrote:
> An alternative solution is to inspect a binary created by clang or rustc
> to see whether the faulty CFI tags are in the binary. This would be a
> precise check, but it would involve hard-coding the *hashed* version of
> the CFI tag. This is because there's no way to get clang or rustc to
> output the unhased version of the CFI tag. Relying on the precise
> hashing algorithm using by CFI seems too fragile, so I have not pursued
> this option.

I suppose there would be no need to hardcode hashes in the test,
it's enough to verify that the hashes for the compiler-emitted
functions change when integer normalization is enabled. Still, I
agree that this doesn't sound worth it in this case.

> diff --git a/arch/Kconfig b/arch/Kconfig
> index ee58df8b1080..b8066bf43153 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -829,7 +829,7 @@ config CFI_CLANG
>  config CFI_ICALL_NORMALIZE_INTEGERS
>  	bool "Normalize CFI tags for integers"
>  	depends on CFI_CLANG
> -	depends on $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)
> +	depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS
>  	help
>  	  This option normalizes the CFI tags for integer types so that all
>  	  integer types of the same size and signedness receive the same CFI
> @@ -842,6 +842,22 @@ config CFI_ICALL_NORMALIZE_INTEGERS
>  
>  	  This option is necessary for using CFI with Rust. If unsure, say N.
>  
> +config HAVE_CFI_ICALL_NORMALIZE_INTEGERS
> +	def_bool !GCOV_KERNEL && !KASAN
> +	depends on CFI_CLANG
> +	depends on $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)

This looks reasonable to me. Thanks for the fix!

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

On Thu, Sep 26, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> Hi Alice,
>
> On Wed, Sep 25, 2024 at 08:10:18AM +0000, Alice Ryhl wrote:
> > An alternative solution is to inspect a binary created by clang or rustc
> > to see whether the faulty CFI tags are in the binary. This would be a
> > precise check, but it would involve hard-coding the *hashed* version of
> > the CFI tag. This is because there's no way to get clang or rustc to
> > output the unhased version of the CFI tag. Relying on the precise
> > hashing algorithm using by CFI seems too fragile, so I have not pursued
> > this option.
>
> I suppose there would be no need to hardcode hashes in the test,
> it's enough to verify that the hashes for the compiler-emitted
> functions change when integer normalization is enabled. Still, I
> agree that this doesn't sound worth it in this case.

Future compilers could change it so that the same hash works in both
cases. After all, the signatures in question have no integers in them.

> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index ee58df8b1080..b8066bf43153 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -829,7 +829,7 @@ config CFI_CLANG
> >  config CFI_ICALL_NORMALIZE_INTEGERS
> >       bool "Normalize CFI tags for integers"
> >       depends on CFI_CLANG
> > -     depends on $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)
> > +     depends on HAVE_CFI_ICALL_NORMALIZE_INTEGERS
> >       help
> >         This option normalizes the CFI tags for integer types so that all
> >         integer types of the same size and signedness receive the same CFI
> > @@ -842,6 +842,22 @@ config CFI_ICALL_NORMALIZE_INTEGERS
> >
> >         This option is necessary for using CFI with Rust. If unsure, say N.
> >
> > +config HAVE_CFI_ICALL_NORMALIZE_INTEGERS
> > +     def_bool !GCOV_KERNEL && !KASAN
> > +     depends on CFI_CLANG
> > +     depends on $(cc-option,-fsanitize=kcfi -fsanitize-cfi-icall-experimental-normalize-integers)
>
> This looks reasonable to me. Thanks for the fix!
>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Thanks for taking a look!

Alice