| Message ID | 20240925-cfi-norm-kasan-fix-v1-1-0328985cdf33@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cfi: encode cfi normalized integers + kasan/gcov bug in Kconfig | expand |
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
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
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
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(-) --- base-commit: a2f11547052001bd448ccec81dd1e68409078fbb change-id: 20240924-cfi-norm-kasan-fix-71332665cc5c Best regards,