[PATCH 0/2] Rust KCFI support

[PATCH 0/2] Rust KCFI support

[Alice Ryhl]

The control flow integrity (kCFI) sanitizer is an important sanitizer
that is often used in production. This patch series makes it possible to
use kCFI and Rust together.

The second patch in this series depends on the next version of [1],
which Miguel will send soon. It also depends on [2].

Link: https://lore.kernel.org/r/20240709160615.998336-12-ojeda@kernel.org [1]
Link: https://lore.kernel.org/r/20240730-target-json-arrays-v1-1-2b376fd0ecf4@google.com [2]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Alice Ryhl (1):
      cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

Matthew Maurer (1):
      rust: cfi: add support for CFI_CLANG with Rust

 Makefile                        | 10 ++++++++++
 arch/Kconfig                    | 11 +++++++++++
 init/Kconfig                    |  2 +-
 rust/Makefile                   |  2 +-
 scripts/generate_rust_target.rs |  1 +
 5 files changed, 24 insertions(+), 2 deletions(-)
---
base-commit: 8718bc07faa6ddf4f7335a12e4cdd4ffc796bbd8
change-id: 20240725-kcfi-c592898e2bfb

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

[PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Alice Ryhl]

Introduce a Kconfig option for enabling the experimental option to
normalize integer types. This ensures that integer types of the same
size and signedness are considered compatible by the Control Flow
Integrity sanitizer.

This option exists for compatibility with Rust, as C and Rust do not
have the same set of integer types. There are cases where C has two
different integer types of the same size and alignment, but Rust only
has one integer type of that size and alignment. When Rust calls into
C functions using such types in their signature, this results in CFI
failures.

This patch introduces a dedicated option for this because it is
undesirable to have CONFIG_RUST affect CC_FLAGS in this way.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 Makefile     |  3 +++
 arch/Kconfig | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/Makefile b/Makefile
index 2b5f9f098b6f..484c6900337e 100644
--- a/Makefile
+++ b/Makefile
@@ -952,6 +952,9 @@ endif
 
 ifdef CONFIG_CFI_CLANG
 CC_FLAGS_CFI	:= -fsanitize=kcfi
+ifdef CONFIG_CFI_ICALL_NORMALIZE_INTEGERS
+	CC_FLAGS_CFI	+= -fsanitize-cfi-icall-experimental-normalize-integers
+endif
 KBUILD_CFLAGS	+= $(CC_FLAGS_CFI)
 export CC_FLAGS_CFI
 endif
diff --git a/arch/Kconfig b/arch/Kconfig
index 975dd22a2dbd..f6ecb15cb8ba 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -826,6 +826,17 @@ config CFI_CLANG
 
 	    https://clang.llvm.org/docs/ControlFlowIntegrity.html
 
+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)
+	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
+	  tag.
+
+	  This option is necessary for using CFI with Rust. If unsure, say N.
+
 config CFI_PERMISSIVE
 	bool "Use CFI in permissive mode"
 	depends on CFI_CLANG

-- 
2.46.0.rc1.232.g9752f9e123-goog

[PATCH 2/2] rust: cfi: add support for CFI_CLANG with Rust

[Alice Ryhl]

From: Matthew Maurer <mmaurer@google.com>

Make it possible to use the Control Flow Integrity (CFI) sanitizer when
Rust is enabled. Enabling CFI with Rust requires that CFI is configured
to normalize integer types so that all integer types of the same size
and signedness are compatible under CFI.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 Makefile                        | 7 +++++++
 init/Kconfig                    | 2 +-
 rust/Makefile                   | 2 +-
 scripts/generate_rust_target.rs | 1 +
 4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 484c6900337e..8d7d52f57c63 100644
--- a/Makefile
+++ b/Makefile
@@ -955,6 +955,13 @@ CC_FLAGS_CFI	:= -fsanitize=kcfi
 ifdef CONFIG_CFI_ICALL_NORMALIZE_INTEGERS
 	CC_FLAGS_CFI	+= -fsanitize-cfi-icall-experimental-normalize-integers
 endif
+ifdef CONFIG_RUST
+	# Always pass -Zsanitizer-cfi-normalize-integers as CONFIG_RUST depends
+	# on CONFIG_CFI_ICALL_NORMALIZE_INTEGERS.
+	RS_FLAGS_CFI   := -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers
+	KBUILD_RUSTFLAGS += $(RS_FLAGS_CFI)
+	export RS_FLAGS_CFI
+endif
 KBUILD_CFLAGS	+= $(CC_FLAGS_CFI)
 export CC_FLAGS_CFI
 endif
diff --git a/init/Kconfig b/init/Kconfig
index b0238c4b6e79..d0d3442d1756 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1905,11 +1905,11 @@ config RUST
 	bool "Rust support"
 	depends on HAVE_RUST
 	depends on RUST_IS_AVAILABLE
-	depends on !CFI_CLANG
 	depends on !MODVERSIONS
 	depends on !GCC_PLUGINS
 	depends on !RANDSTRUCT
 	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
+	depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && CFI_ICALL_NORMALIZE_INTEGERS
 	help
 	  Enables Rust support in the kernel.
 
diff --git a/rust/Makefile b/rust/Makefile
index f6b9bb946609..a2c9a3e03a23 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -305,7 +305,7 @@ $(obj)/bindings/bindings_helpers_generated.rs: $(src)/helpers.c FORCE
 quiet_cmd_exports = EXPORTS $@
       cmd_exports = \
 	$(NM) -p --defined-only $< \
-		| awk '/ (T|R|D) / {printf "EXPORT_SYMBOL_RUST_GPL(%s);\n",$$3}' > $@
+		| awk '$$2~/(T|R|D)/ && $$3!~/__cfi/ {printf "EXPORT_SYMBOL_RUST_GPL(%s);\n",$$3}' > $@
 
 $(obj)/exports_core_generated.h: $(obj)/core.o FORCE
 	$(call if_changed,exports)
diff --git a/scripts/generate_rust_target.rs b/scripts/generate_rust_target.rs
index c31657380bf9..9b184099278a 100644
--- a/scripts/generate_rust_target.rs
+++ b/scripts/generate_rust_target.rs
@@ -192,6 +192,7 @@ fn main() {
         }
         ts.push("features", features);
         ts.push("llvm-target", "x86_64-linux-gnu");
+        ts.push("supported-sanitizers", ["kcfi"]);
         ts.push("target-pointer-width", "64");
     } else if cfg.has("X86_32") {
         // This only works on UML, as i386 otherwise needs regparm support in rustc

-- 
2.46.0.rc1.232.g9752f9e123-goog

Re: [PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Alice Ryhl]

On Tue, Jul 30, 2024 at 11:40 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Introduce a Kconfig option for enabling the experimental option to
> normalize integer types. This ensures that integer types of the same
> size and signedness are considered compatible by the Control Flow
> Integrity sanitizer.
>
> This option exists for compatibility with Rust, as C and Rust do not
> have the same set of integer types. There are cases where C has two
> different integer types of the same size and alignment, but Rust only
> has one integer type of that size and alignment. When Rust calls into
> C functions using such types in their signature, this results in CFI
> failures.

This should say signedness where it says alignment.

Alice

Re: [PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Peter Zijlstra]

On Tue, Jul 30, 2024 at 09:40:11AM +0000, Alice Ryhl wrote:
> Introduce a Kconfig option for enabling the experimental option to
> normalize integer types. This ensures that integer types of the same
> size and signedness are considered compatible by the Control Flow
> Integrity sanitizer.
> 
> This option exists for compatibility with Rust, as C and Rust do not
> have the same set of integer types. There are cases where C has two
> different integer types of the same size and alignment, but Rust only
> has one integer type of that size and alignment. When Rust calls into
> C functions using such types in their signature, this results in CFI
> failures.
> 
> This patch introduces a dedicated option for this because it is
> undesirable to have CONFIG_RUST affect CC_FLAGS in this way.

To be clear, any code compiled with this is incompatible with code
compiled without this, as the function signatures will differ, right?

Specifically, it will map things like 'unsigned long long' and 'unsigned
long' -- which are both u64 on LP64 targets to the same 'type', right?

I suppose it has been decided the security impact of this change is
minimal?

All in all, there is very little actual information provided here.

> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  Makefile     |  3 +++
>  arch/Kconfig | 11 +++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 2b5f9f098b6f..484c6900337e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -952,6 +952,9 @@ endif
>  
>  ifdef CONFIG_CFI_CLANG
>  CC_FLAGS_CFI	:= -fsanitize=kcfi
> +ifdef CONFIG_CFI_ICALL_NORMALIZE_INTEGERS
> +	CC_FLAGS_CFI	+= -fsanitize-cfi-icall-experimental-normalize-integers
> +endif
>  KBUILD_CFLAGS	+= $(CC_FLAGS_CFI)
>  export CC_FLAGS_CFI
>  endif
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 975dd22a2dbd..f6ecb15cb8ba 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -826,6 +826,17 @@ config CFI_CLANG
>  
>  	    https://clang.llvm.org/docs/ControlFlowIntegrity.html
>  
> +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)
> +	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
> +	  tag.
> +
> +	  This option is necessary for using CFI with Rust. If unsure, say N.
> +
>  config CFI_PERMISSIVE
>  	bool "Use CFI in permissive mode"
>  	depends on CFI_CLANG
> 
> -- 
> 2.46.0.rc1.232.g9752f9e123-goog
> 

Re: [PATCH 2/2] rust: cfi: add support for CFI_CLANG with Rust

[Peter Zijlstra]

On Tue, Jul 30, 2024 at 09:40:12AM +0000, Alice Ryhl wrote:
> From: Matthew Maurer <mmaurer@google.com>
> 
> Make it possible to use the Control Flow Integrity (CFI) sanitizer when
> Rust is enabled. Enabling CFI with Rust requires that CFI is configured
> to normalize integer types so that all integer types of the same size
> and signedness are compatible under CFI.

I am assuming -- because I have to, because you're not actually saying
anyting -- that this is fully compatible with the C version and all the
fun and games we play with rewriting the function prologue for FineIBT
and the like also work?

> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  Makefile                        | 7 +++++++
>  init/Kconfig                    | 2 +-
>  rust/Makefile                   | 2 +-
>  scripts/generate_rust_target.rs | 1 +
>  4 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 484c6900337e..8d7d52f57c63 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -955,6 +955,13 @@ CC_FLAGS_CFI	:= -fsanitize=kcfi
>  ifdef CONFIG_CFI_ICALL_NORMALIZE_INTEGERS
>  	CC_FLAGS_CFI	+= -fsanitize-cfi-icall-experimental-normalize-integers
>  endif
> +ifdef CONFIG_RUST
> +	# Always pass -Zsanitizer-cfi-normalize-integers as CONFIG_RUST depends
> +	# on CONFIG_CFI_ICALL_NORMALIZE_INTEGERS.
> +	RS_FLAGS_CFI   := -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers
> +	KBUILD_RUSTFLAGS += $(RS_FLAGS_CFI)
> +	export RS_FLAGS_CFI
> +endif
>  KBUILD_CFLAGS	+= $(CC_FLAGS_CFI)
>  export CC_FLAGS_CFI
>  endif
> diff --git a/init/Kconfig b/init/Kconfig
> index b0238c4b6e79..d0d3442d1756 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1905,11 +1905,11 @@ config RUST
>  	bool "Rust support"
>  	depends on HAVE_RUST
>  	depends on RUST_IS_AVAILABLE
> -	depends on !CFI_CLANG
>  	depends on !MODVERSIONS
>  	depends on !GCC_PLUGINS
>  	depends on !RANDSTRUCT
>  	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
> +	depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && CFI_ICALL_NORMALIZE_INTEGERS
>  	help
>  	  Enables Rust support in the kernel.
>  
> diff --git a/rust/Makefile b/rust/Makefile
> index f6b9bb946609..a2c9a3e03a23 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -305,7 +305,7 @@ $(obj)/bindings/bindings_helpers_generated.rs: $(src)/helpers.c FORCE
>  quiet_cmd_exports = EXPORTS $@
>        cmd_exports = \
>  	$(NM) -p --defined-only $< \
> -		| awk '/ (T|R|D) / {printf "EXPORT_SYMBOL_RUST_GPL(%s);\n",$$3}' > $@
> +		| awk '$$2~/(T|R|D)/ && $$3!~/__cfi/ {printf "EXPORT_SYMBOL_RUST_GPL(%s);\n",$$3}' > $@
>  
>  $(obj)/exports_core_generated.h: $(obj)/core.o FORCE
>  	$(call if_changed,exports)
> diff --git a/scripts/generate_rust_target.rs b/scripts/generate_rust_target.rs
> index c31657380bf9..9b184099278a 100644
> --- a/scripts/generate_rust_target.rs
> +++ b/scripts/generate_rust_target.rs
> @@ -192,6 +192,7 @@ fn main() {
>          }
>          ts.push("features", features);
>          ts.push("llvm-target", "x86_64-linux-gnu");
> +        ts.push("supported-sanitizers", ["kcfi"]);
>          ts.push("target-pointer-width", "64");
>      } else if cfg.has("X86_32") {
>          // This only works on UML, as i386 otherwise needs regparm support in rustc
> 
> -- 
> 2.46.0.rc1.232.g9752f9e123-goog
> 

Re: [PATCH 0/2] Rust KCFI support

[Gary Guo]

On Tue, 30 Jul 2024 09:40:10 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> The control flow integrity (kCFI) sanitizer is an important sanitizer
> that is often used in production. This patch series makes it possible to
> use kCFI and Rust together.
> 
> The second patch in this series depends on the next version of [1],
> which Miguel will send soon. It also depends on [2].
> 
> Link: https://lore.kernel.org/r/20240709160615.998336-12-ojeda@kernel.org [1]
> Link: https://lore.kernel.org/r/20240730-target-json-arrays-v1-1-2b376fd0ecf4@google.com [2]
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Both patches:

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
> Alice Ryhl (1):
>       cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS
> 
> Matthew Maurer (1):
>       rust: cfi: add support for CFI_CLANG with Rust
> 
>  Makefile                        | 10 ++++++++++
>  arch/Kconfig                    | 11 +++++++++++
>  init/Kconfig                    |  2 +-
>  rust/Makefile                   |  2 +-
>  scripts/generate_rust_target.rs |  1 +
>  5 files changed, 24 insertions(+), 2 deletions(-)
> ---
> base-commit: 8718bc07faa6ddf4f7335a12e4cdd4ffc796bbd8
> change-id: 20240725-kcfi-c592898e2bfb
> 
> Best regards,

Re: [PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Miguel Ojeda]

On Tue, Jul 30, 2024 at 11:40 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Introduce a Kconfig option for enabling the experimental option to
> normalize integer types. This ensures that integer types of the same
> size and signedness are considered compatible by the Control Flow
> Integrity sanitizer.
>
> This option exists for compatibility with Rust, as C and Rust do not
> have the same set of integer types. There are cases where C has two
> different integer types of the same size and alignment, but Rust only
> has one integer type of that size and alignment. When Rust calls into
> C functions using such types in their signature, this results in CFI
> failures.
>
> This patch introduces a dedicated option for this because it is
> undesirable to have CONFIG_RUST affect CC_FLAGS in this way.

Is there any case where we would want CFI_ICALL_NORMALIZE_INTEGERS
when Rust is not enabled, then? If not, is the idea here to make this
an explicit extra question in the config before enabling Rust? Or why
wouldn't it be done automatically?

Thanks!

Cheers,
Miguel

Re: [PATCH 2/2] rust: cfi: add support for CFI_CLANG with Rust

[Miguel Ojeda]

On Tue, Jul 30, 2024 at 11:40 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> +       RS_FLAGS_CFI   := -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers

Before I forget: this should probably be `RUSTC_...` for consistency
with the rest (and, in this case, these are flags, so it makes sense
they target the particular compiler).

Cheers,
Miguel

Re: [PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Peter Zijlstra]

On Tue, Jul 30, 2024 at 01:38:33PM +0200, Miguel Ojeda wrote:
> On Tue, Jul 30, 2024 at 11:40 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > Introduce a Kconfig option for enabling the experimental option to
> > normalize integer types. This ensures that integer types of the same
> > size and signedness are considered compatible by the Control Flow
> > Integrity sanitizer.
> >
> > This option exists for compatibility with Rust, as C and Rust do not
> > have the same set of integer types. There are cases where C has two
> > different integer types of the same size and alignment, but Rust only
> > has one integer type of that size and alignment. When Rust calls into
> > C functions using such types in their signature, this results in CFI
> > failures.
> >
> > This patch introduces a dedicated option for this because it is
> > undesirable to have CONFIG_RUST affect CC_FLAGS in this way.
> 
> Is there any case where we would want CFI_ICALL_NORMALIZE_INTEGERS
> when Rust is not enabled, then? If not, is the idea here to make this
> an explicit extra question in the config before enabling Rust? Or why
> wouldn't it be done automatically?

I suspect CFI_ICALL_NORMALIZE_INTEGERS breaks ABI, then again, Linux
doesn't promise or preserve ABI except for the SYSCALL layer. So yeah,
meh.

Re: [PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Sami Tolvanen]

On Tue, Jul 30, 2024 at 3:29 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jul 30, 2024 at 09:40:11AM +0000, Alice Ryhl wrote:
> > Introduce a Kconfig option for enabling the experimental option to
> > normalize integer types. This ensures that integer types of the same
> > size and signedness are considered compatible by the Control Flow
> > Integrity sanitizer.
> >
> > This option exists for compatibility with Rust, as C and Rust do not
> > have the same set of integer types. There are cases where C has two
> > different integer types of the same size and alignment, but Rust only
> > has one integer type of that size and alignment. When Rust calls into
> > C functions using such types in their signature, this results in CFI
> > failures.
> >
> > This patch introduces a dedicated option for this because it is
> > undesirable to have CONFIG_RUST affect CC_FLAGS in this way.
>
> To be clear, any code compiled with this is incompatible with code
> compiled without this, as the function signatures will differ, right?
>
> Specifically, it will map things like 'unsigned long long' and 'unsigned
> long' -- which are both u64 on LP64 targets to the same 'type', right?
>
> I suppose it has been decided the security impact of this change is
> minimal?

I looked into this last year, and integer normalization reduced the
number of unique type hashes in the kernel by ~1%, which should be
fine.

Sami

Re: [PATCH 2/2] rust: cfi: add support for CFI_CLANG with Rust

[Sami Tolvanen]

On Tue, Jul 30, 2024 at 3:32 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jul 30, 2024 at 09:40:12AM +0000, Alice Ryhl wrote:
> > From: Matthew Maurer <mmaurer@google.com>
> >
> > Make it possible to use the Control Flow Integrity (CFI) sanitizer when
> > Rust is enabled. Enabling CFI with Rust requires that CFI is configured
> > to normalize integer types so that all integer types of the same size
> > and signedness are compatible under CFI.
>
> I am assuming -- because I have to, because you're not actually saying
> anyting -- that this is fully compatible with the C version and all the
> fun and games we play with rewriting the function prologue for FineIBT
> and the like also work?

Rust uses the same LLVM backend for the actual code generation, so it
should be fully compatible.

Sami

Re: [PATCH 2/2] rust: cfi: add support for CFI_CLANG with Rust

[Peter Zijlstra]

On Tue, Jul 30, 2024 at 08:24:15AM -0700, Sami Tolvanen wrote:
> On Tue, Jul 30, 2024 at 3:32 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Tue, Jul 30, 2024 at 09:40:12AM +0000, Alice Ryhl wrote:
> > > From: Matthew Maurer <mmaurer@google.com>
> > >
> > > Make it possible to use the Control Flow Integrity (CFI) sanitizer when
> > > Rust is enabled. Enabling CFI with Rust requires that CFI is configured
> > > to normalize integer types so that all integer types of the same size
> > > and signedness are compatible under CFI.
> >
> > I am assuming -- because I have to, because you're not actually saying
> > anyting -- that this is fully compatible with the C version and all the
> > fun and games we play with rewriting the function prologue for FineIBT
> > and the like also work?
> 
> Rust uses the same LLVM backend for the actual code generation, so it
> should be fully compatible.

Yes, but we also combine that with -fpatchable-function-entry= for a
very specific effect, and I don't think I see the Rust thingy do that.

Re: [PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Peter Zijlstra]

On Tue, Jul 30, 2024 at 08:19:13AM -0700, Sami Tolvanen wrote:
> On Tue, Jul 30, 2024 at 3:29 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Tue, Jul 30, 2024 at 09:40:11AM +0000, Alice Ryhl wrote:
> > > Introduce a Kconfig option for enabling the experimental option to
> > > normalize integer types. This ensures that integer types of the same
> > > size and signedness are considered compatible by the Control Flow
> > > Integrity sanitizer.
> > >
> > > This option exists for compatibility with Rust, as C and Rust do not
> > > have the same set of integer types. There are cases where C has two
> > > different integer types of the same size and alignment, but Rust only
> > > has one integer type of that size and alignment. When Rust calls into
> > > C functions using such types in their signature, this results in CFI
> > > failures.
> > >
> > > This patch introduces a dedicated option for this because it is
> > > undesirable to have CONFIG_RUST affect CC_FLAGS in this way.
> >
> > To be clear, any code compiled with this is incompatible with code
> > compiled without this, as the function signatures will differ, right?
> >
> > Specifically, it will map things like 'unsigned long long' and 'unsigned
> > long' -- which are both u64 on LP64 targets to the same 'type', right?
> >
> > I suppose it has been decided the security impact of this change is
> > minimal?
> 
> I looked into this last year, and integer normalization reduced the
> number of unique type hashes in the kernel by ~1%, which should be
> fine.

Right, I didn't actually expect it to matter much either, but it
would've made good Changelog content.

Re: [PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Alice Ryhl]

On Tue, Jul 30, 2024 at 12:29 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jul 30, 2024 at 09:40:11AM +0000, Alice Ryhl wrote:
> > Introduce a Kconfig option for enabling the experimental option to
> > normalize integer types. This ensures that integer types of the same
> > size and signedness are considered compatible by the Control Flow
> > Integrity sanitizer.
> >
> > This option exists for compatibility with Rust, as C and Rust do not
> > have the same set of integer types. There are cases where C has two
> > different integer types of the same size and alignment, but Rust only
> > has one integer type of that size and alignment. When Rust calls into
> > C functions using such types in their signature, this results in CFI
> > failures.
> >
> > This patch introduces a dedicated option for this because it is
> > undesirable to have CONFIG_RUST affect CC_FLAGS in this way.
>
> To be clear, any code compiled with this is incompatible with code
> compiled without this, as the function signatures will differ, right?

That's correct.

> Specifically, it will map things like 'unsigned long long' and 'unsigned
> long' -- which are both u64 on LP64 targets to the same 'type', right?

That's correct.

> I suppose it has been decided the security impact of this change is
> minimal?

Yeah, see Sami's response.

> All in all, there is very little actual information provided here.

I'll make sure to include this info in the commit message of v2.

Alice

Re: [PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Alice Ryhl]

On Tue, Jul 30, 2024 at 1:38 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Jul 30, 2024 at 11:40 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > Introduce a Kconfig option for enabling the experimental option to
> > normalize integer types. This ensures that integer types of the same
> > size and signedness are considered compatible by the Control Flow
> > Integrity sanitizer.
> >
> > This option exists for compatibility with Rust, as C and Rust do not
> > have the same set of integer types. There are cases where C has two
> > different integer types of the same size and alignment, but Rust only
> > has one integer type of that size and alignment. When Rust calls into
> > C functions using such types in their signature, this results in CFI
> > failures.
> >
> > This patch introduces a dedicated option for this because it is
> > undesirable to have CONFIG_RUST affect CC_FLAGS in this way.
>
> Is there any case where we would want CFI_ICALL_NORMALIZE_INTEGERS
> when Rust is not enabled, then? If not, is the idea here to make this
> an explicit extra question in the config before enabling Rust? Or why
> wouldn't it be done automatically?

I'm adding this flag to make the bringup process for RUST easier.

I'm working on enabling RUST in a new branch. We're eventually going
to have both RUST and CFI_ICALL_NORMALIZE_INTEGERS enabled in our
build, but the path to getting there is complex and we would like to
turn on CFI_ICALL_NORMALIZE_INTEGERS first, and then turn on RUST
later. Both options are non-trivial to turn on and I want to
disentangle them.

Alice

Re: [PATCH 2/2] rust: cfi: add support for CFI_CLANG with Rust

[Alice Ryhl]

On Tue, Jul 30, 2024 at 6:03 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jul 30, 2024 at 08:24:15AM -0700, Sami Tolvanen wrote:
> > On Tue, Jul 30, 2024 at 3:32 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Tue, Jul 30, 2024 at 09:40:12AM +0000, Alice Ryhl wrote:
> > > > From: Matthew Maurer <mmaurer@google.com>
> > > >
> > > > Make it possible to use the Control Flow Integrity (CFI) sanitizer when
> > > > Rust is enabled. Enabling CFI with Rust requires that CFI is configured
> > > > to normalize integer types so that all integer types of the same size
> > > > and signedness are compatible under CFI.
> > >
> > > I am assuming -- because I have to, because you're not actually saying
> > > anyting -- that this is fully compatible with the C version and all the
> > > fun and games we play with rewriting the function prologue for FineIBT
> > > and the like also work?
> >
> > Rust uses the same LLVM backend for the actual code generation, so it
> > should be fully compatible.
>
> Yes, but we also combine that with -fpatchable-function-entry= for a
> very specific effect, and I don't think I see the Rust thingy do that.

Oh, you're right. I missed this because we're not using FineIBT, and
when this patch was originally written we had not yet implemented a
Rust equivalent to -fpatchable-function-entry=. However, that flag is
now available as of rustc version 1.80.0, so it shouldn't be an issue.
I'll fix this for v2 of this series.

Thanks for catching this!

As for whether it works other than that, I've been using this on my
personal phone together with Rust Binder for several months, so it's
well tested on arm64.

Alice

Re: [PATCH 2/2] rust: cfi: add support for CFI_CLANG with Rust

[Alice Ryhl]

On Tue, Jul 30, 2024 at 1:51 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Jul 30, 2024 at 11:40 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > +       RS_FLAGS_CFI   := -Zsanitizer=kcfi -Zsanitizer-cfi-normalize-integers
>
> Before I forget: this should probably be `RUSTC_...` for consistency
> with the rest (and, in this case, these are flags, so it makes sense
> they target the particular compiler).

Hmm. It seems like the existing variables containing rustc flags just
use RUST not RUSTC in the name?

Alice

Re: [PATCH 2/2] rust: cfi: add support for CFI_CLANG with Rust

[Miguel Ojeda]

On Tue, Jul 30, 2024 at 6:44 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Hmm. It seems like the existing variables containing rustc flags just
> use RUST not RUSTC in the name?

If you mean things like RUSTFLAGS, it was because the C side uses
CFLAGS, but for the equivalent of CC_FLAGS or CLANG_FLAGS, I would say
it should be RUSTC_FLAGS.

Cheers,
Miguel

Re: [PATCH 1/2] cfi: add CONFIG_CFI_ICALL_NORMALIZE_INTEGERS

[Miguel Ojeda]

On Tue, Jul 30, 2024 at 6:10 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> I'm adding this flag to make the bringup process for RUST easier.
>
> I'm working on enabling RUST in a new branch. We're eventually going
> to have both RUST and CFI_ICALL_NORMALIZE_INTEGERS enabled in our
> build, but the path to getting there is complex and we would like to
> turn on CFI_ICALL_NORMALIZE_INTEGERS first, and then turn on RUST
> later. Both options are non-trivial to turn on and I want to
> disentangle them.

Would it be useful for other users/distros to do that two-stage
approach as well?

In other words, if the intended end state is that everybody should
enable this if Rust is enabled, and nobody should enable it if they
don't care about Rust, then we should add this only if you think
others will also need to do this step by step. The option or commit
message could ideally explain more about this need/use case.

Cheers,
Miguel