| Message ID | 20240730-kcfi-v1-1-bbb948752a30@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rust KCFI support | expand |
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
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 >
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
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.
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
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.
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
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
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
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
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(+)