| Message ID | 20240404-providing-emporium-e652e359c711@spud (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v3] rust: make mutually exclusive with CFI_CLANG | expand |
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
> > 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
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 > >
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 :)
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
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
> > 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.
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
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
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