[PATCH v3] rust: make mutually exclusive with CFI_CLANG

[PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

On RISC-V and arm64, and presumably x86, if CFI_CLANG is enabled,
loading a rust module will trigger a kernel panic. Support for
sanitisers, including kcfi (CFI_CLANG), is in the works, but for now
they're nightly-only options in rustc. Make RUST depend on !CFI_CLANG
to prevent configuring a kernel without symmetrical support for kfi.

Fixes: 2f7ab1267dc9 ("Kbuild: add Rust support")
cc: stable@vger.kernel.org
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Sending this one on its own, there's no explicit dep on this for the
riscv enabling patch, v3 to continue the numbering from there. Nothing
has changed since v2.

CC: Miguel Ojeda <ojeda@kernel.org>
CC: Alex Gaynor <alex.gaynor@gmail.com>
CC: Wedson Almeida Filho <wedsonaf@gmail.com>
CC: linux-kernel@vger.kernel.org (open list)
CC: rust-for-linux@vger.kernel.org
CC: Sami Tolvanen <samitolvanen@google.com>
CC: Kees Cook <keescook@chromium.org>
CC: Nathan Chancellor <nathan@kernel.org>
CC: llvm@lists.linux.dev
---
 init/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/init/Kconfig b/init/Kconfig
index aa02aec6aa7d..ad9a2da27dc9 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1899,6 +1899,7 @@ 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
-- 
2.43.0

Re: [PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Miguel Ojeda]

On Thu, Apr 4, 2024 at 4:17 PM Conor Dooley <conor@kernel.org> wrote:
>
> From: Conor Dooley <conor.dooley@microchip.com>
>
> On RISC-V and arm64, and presumably x86, if CFI_CLANG is enabled,
> loading a rust module will trigger a kernel panic. Support for
> sanitisers, including kcfi (CFI_CLANG), is in the works, but for now
> they're nightly-only options in rustc. Make RUST depend on !CFI_CLANG
> to prevent configuring a kernel without symmetrical support for kfi.
>
> Fixes: 2f7ab1267dc9 ("Kbuild: add Rust support")
> cc: stable@vger.kernel.org
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Sending this one on its own, there's no explicit dep on this for the
> riscv enabling patch, v3 to continue the numbering from there. Nothing
> has changed since v2.
>
> CC: Miguel Ojeda <ojeda@kernel.org>
> CC: Alex Gaynor <alex.gaynor@gmail.com>
> CC: Wedson Almeida Filho <wedsonaf@gmail.com>
> CC: linux-kernel@vger.kernel.org (open list)
> CC: rust-for-linux@vger.kernel.org
> CC: Sami Tolvanen <samitolvanen@google.com>
> CC: Kees Cook <keescook@chromium.org>
> CC: Nathan Chancellor <nathan@kernel.org>
> CC: llvm@lists.linux.dev

Cc'ing Matthew & Ramon as well so that they are aware and in case they
want to comment.

Cheers,
Miguel

Re: [PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Matthew Maurer]

>
> Cc'ing Matthew & Ramon as well so that they are aware and in case they
> want to comment.
>
> Cheers,
> Miguel

This patch is fine by me - the last patch needed for KCFI to be
functional in Rust just landed upstream last night, so we should
revisit this (in the form of enabling it) once we move to
`rustc-1.79.0` or later. In case anyone wants it for local
experimentation, I have a backport branch [1] which applies these to
the 1.73.0 compiler and enables it in the kernel [2] (not upstreamed
because the feature isn't yet in kernel's `rustc`), which Android will
be using for the Rust binder driver. This patch will require a recent
(last year or so) clang, as it relies on
`-fsanitize-cfi-icall-experimental-normalize-integers`.

[1]: https://github.com/maurer/rust/tree/1.73.0%2Bcfi
[2]: https://android-review.git.corp.google.com/c/kernel/common/+/2930616

Re: [PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Nathan Chancellor]

On Thu, Apr 04, 2024 at 03:17:02PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> On RISC-V and arm64, and presumably x86, if CFI_CLANG is enabled,
> loading a rust module will trigger a kernel panic. Support for
> sanitisers, including kcfi (CFI_CLANG), is in the works, but for now
> they're nightly-only options in rustc. Make RUST depend on !CFI_CLANG
> to prevent configuring a kernel without symmetrical support for kfi.
> 
> Fixes: 2f7ab1267dc9 ("Kbuild: add Rust support")
> cc: stable@vger.kernel.org
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Acked-by: Nathan Chancellor <nathan@kernel.org>

It seems like this won't be forgotten about but if there is not already
an issue open for this somewhere, it would be good to have one, since we
obviously want this for both C and Rust code.

As a general meta comment not directed at anyone in particualr, I think
these 'depends on !' should all have some sort of comment or description
as to why they are disabled. I can infer from most of them but it would
still be good to be explicit, especially since someone might want to
work on fixing the ones that are due to missing support and such.

> ---
> Sending this one on its own, there's no explicit dep on this for the
> riscv enabling patch, v3 to continue the numbering from there. Nothing
> has changed since v2.
> 
> CC: Miguel Ojeda <ojeda@kernel.org>
> CC: Alex Gaynor <alex.gaynor@gmail.com>
> CC: Wedson Almeida Filho <wedsonaf@gmail.com>
> CC: linux-kernel@vger.kernel.org (open list)
> CC: rust-for-linux@vger.kernel.org
> CC: Sami Tolvanen <samitolvanen@google.com>
> CC: Kees Cook <keescook@chromium.org>
> CC: Nathan Chancellor <nathan@kernel.org>
> CC: llvm@lists.linux.dev
> ---
>  init/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index aa02aec6aa7d..ad9a2da27dc9 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1899,6 +1899,7 @@ 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
> -- 
> 2.43.0
> 
> 

Re: [PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Thu, Apr 04, 2024 at 08:25:11AM -0700, Matthew Maurer wrote:
> >
> > Cc'ing Matthew & Ramon as well so that they are aware and in case they
> > want to comment.

Ah crap, I knew I forgot to CC someone from the last conversation,
thanks Miguel.

> This patch is fine by me - the last patch needed for KCFI to be
> functional in Rust just landed upstream last night, so we should
> revisit this (in the form of enabling it) once we move to
> `rustc-1.79.0` or later.

That's great news, thanks for working on it :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Miguel Ojeda]

On Thu, Apr 4, 2024 at 5:25 PM Matthew Maurer <mmaurer@google.com> wrote:
>
> This patch is fine by me - the last patch needed for KCFI to be
> functional in Rust just landed upstream last night, so we should
> revisit this (in the form of enabling it) once we move to
> `rustc-1.79.0` or later. In case anyone wants it for local
> experimentation, I have a backport branch [1] which applies these to
> the 1.73.0 compiler and enables it in the kernel [2] (not upstreamed
> because the feature isn't yet in kernel's `rustc`), which Android will
> be using for the Rust binder driver. This patch will require a recent
> (last year or so) clang, as it relies on
> `-fsanitize-cfi-icall-experimental-normalize-integers`.
>
> [1]: https://github.com/maurer/rust/tree/1.73.0%2Bcfi
> [2]: https://android-review.git.corp.google.com/c/kernel/common/+/2930616

Thanks for the update, Matthew!

I guess the public link is:
https://android-review.googlesource.com/c/kernel/common/+/2930616

Cheers,
Miguel

Re: [PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Miguel Ojeda]

On Thu, Apr 4, 2024 at 5:33 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> Acked-by: Nathan Chancellor <nathan@kernel.org>

Thanks!

> It seems like this won't be forgotten about but if there is not already
> an issue open for this somewhere, it would be good to have one, since we
> obviously want this for both C and Rust code.

We track the unstable feature(s) at
https://github.com/Rust-for-Linux/linux/issues/2 (I just moved this
one there since it is close to ready, but it was in #355 previously,
and cleaned things up a bit).

On the Rust side, I think the main one is
https://github.com/rust-lang/rust/issues/89653.

It includes KCFI, but it is missing Matthew's PRs for KCFI etc. I
added a link to get those issues/PRs (or hopefully most of them):
https://github.com/rust-lang/rust/pulls?q=is%3Apr+cfi+label%3APG-exploit-mitigations+-label%3Arollup

We could create an explicit/concrete issue for removing the `depends
on !` when Matthew sends the patches, though.

> As a general meta comment not directed at anyone in particualr, I think
> these 'depends on !' should all have some sort of comment or description
> as to why they are disabled. I can infer from most of them but it would
> still be good to be explicit, especially since someone might want to
> work on fixing the ones that are due to missing support and such.

That is definitely a good idea. An alternative could be linking an
issue instead (perhaps concrete ones for that, i.e. your other idea
above) so that we can update the status etc. easily.

Cheers,
Miguel

Re: [PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Ramon de C Valle]

>
> We track the unstable feature(s) at
> https://github.com/Rust-for-Linux/linux/issues/2 (I just moved this
> one there since it is close to ready, but it was in #355 previously,
> and cleaned things up a bit).

Sorry about that, I should've done this already. In addition to the
link you added above, here's a tracking issue for KCFI support for
Rust:
https://github.com/rust-lang/rust/issues/123479

There are still a few unresolved questions that I'd like to hear from
you as you experiment with it, which the tl,dr. is: KCFI support for
Rust shares most of its implementation with the CFI support, with some
key differences:

KCFI performs type tests differently and are implemented as different
LLVM passes than CFI to not require LTO.
KCFI has the limitation that a function or method may have one type id
assigned only.

Because of limitation listed above (2), the current KCFI
implementation (not CFI) does reifying of types (i.e., adds
shims/trampolines for indirect calls in these cases) for:

Supporting casting between function items, closures, and Fn trait objects.
Supporting methods being cast as function pointers.

There may be possible costs of these added levels of indirections for
KCFI for cache coherence/locality and performance, possible
introduction of gadgets or KCFI bypasses, and increased
artifact/binary sizes, which haven't been looked at yet, and I'm
honestly not super happy about it (but it's better than the original
proposal of adding shims/trampolines to every virtual call in the Rust
compiler), so I'd be interested in any performance results or
regressions you may have while experimenting with it, more
specifically:

Linux Kernel without KCFI vs Linux Kernel with KCFI vs Linux Kernel
with Rust and KCFI, possibly isolated to the code of a driver that has
an implementation in C vs an alternative implementation in Rust.

Re: [PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Miguel Ojeda]

On Fri, Apr 5, 2024 at 12:34 AM Ramon de C Valle <rcvalle@google.com> wrote:
>
> Sorry about that, I should've done this already. In addition to the link you added above, here's a tracking issue for KCFI support for Rust:
> https://github.com/rust-lang/rust/issues/123479

No worries! Thanks for taking the time creating the new tracking issue
-- linked now from our issue #2.

I also linked your update here and noted the benchmarking needs /
regressions report request too.

Cheers,
Miguel

Re: [PATCH v3] rust: make mutually exclusive with CFI_CLANG

[Miguel Ojeda]

On Thu, Apr 4, 2024 at 4:17 PM Conor Dooley <conor@kernel.org> wrote:
>
> From: Conor Dooley <conor.dooley@microchip.com>
>
> On RISC-V and arm64, and presumably x86, if CFI_CLANG is enabled,
> loading a rust module will trigger a kernel panic. Support for
> sanitisers, including kcfi (CFI_CLANG), is in the works, but for now
> they're nightly-only options in rustc. Make RUST depend on !CFI_CLANG
> to prevent configuring a kernel without symmetrical support for kfi.
>
> Fixes: 2f7ab1267dc9 ("Kbuild: add Rust support")
> cc: stable@vger.kernel.org
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

[ Matthew Maurer writes [1]:

    This patch is fine by me - the last patch needed for KCFI to be
    functional in Rust just landed upstream last night, so we should
    revisit this (in the form of enabling it) once we move to
    `rustc-1.79.0` or later.

  Ramon de C Valle also gave feedback [2] on the status of KCFI for
  Rust and created a tracking issue [3] in upstream Rust.   - Miguel ]

[ Added feedback from the list, links, and used Cc for the tag. ]

Applied to `rust-fixes` -- thanks everyone! Please feel free to send
more tags for this one.

Cheers,
Miguel