Message ID | 20240305-shadow-call-stack-v2-1-c7b4a3f4d616@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] rust: add flags for shadow call stack sanitizer | expand |
On Tue, Mar 5, 2024 at 12:58 PM Alice Ryhl <aliceryhl@google.com> wrote: > > Add flags to support the shadow call stack sanitizer, both in the > dynamic and non-dynamic modes. > > Right now, the compiler will emit the warning "unknown feature specified > for `-Ctarget-feature`: `reserve-x18`". However, the compiler still > passes it to the codegen backend, so the flag will work just fine. Once > rustc starts recognizing the flag (or provides another way to enable the > feature), it will stop emitting this warning. See [1] for the relevant > issue. > > Currently, the compiler thinks that the aarch64-unknown-none target > doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if > you enable shadow call stack in non-dynamic mode. However, I still think > it is reasonable to add the flag now, as it will at least fail the build > when using an invalid configuration, until the Rust compiler is fixed to > list -Zsanitizer=shadow-call-stack as supported for the target. See [2] > for the feature request to add this. > > I have tested this change with Rust Binder on an Android device using > CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the > phone crashes immediately on boot, and with the flag, the phone appears > to work normally. > > This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag > defaults to n, so it isn't a problem today, but the flag is unstable, so > the default could change in a future compiler release. > > Link: https://github.com/rust-lang/rust/issues/121970 [1] > Link: https://github.com/rust-lang/rust/issues/121972 [2] > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > This patch raises the question of whether we should change the Rust > aarch64 support to use a custom target.json specification. If we do > that, then we can fix both the warning for dynamic SCS and the > build-failure for non-dynamic SCS without waiting for a new version of > rustc with the mentioned issues fixed. If the arm64 maintainers are OK with the warning being triggered in that case: Acked-by: Miguel Ojeda <ojeda@kernel.org> Otherwise partially reverting to the `target.json` approach sounds good too. I added the `-Zuse-sync-unwind=n` to the list at https://github.com/Rust-for-Linux/linux/issues/2. Given the default is what we want, I have put it in the "Good to have" section. Cheers, Miguel
> Add flags to support the shadow call stack sanitizer, both in the > dynamic and non-dynamic modes. > > Right now, the compiler will emit the warning "unknown feature specified > for `-Ctarget-feature`: `reserve-x18`". However, the compiler still > passes it to the codegen backend, so the flag will work just fine. Once > rustc starts recognizing the flag (or provides another way to enable the > feature), it will stop emitting this warning. See [1] for the relevant > issue. > > Currently, the compiler thinks that the aarch64-unknown-none target > doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if > you enable shadow call stack in non-dynamic mode. However, I still think > it is reasonable to add the flag now, as it will at least fail the build > when using an invalid configuration, until the Rust compiler is fixed to > list -Zsanitizer=shadow-call-stack as supported for the target. See [2] > for the feature request to add this. > > I have tested this change with Rust Binder on an Android device using > CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the > phone crashes immediately on boot, and with the flag, the phone appears > to work normally. > > This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag > defaults to n, so it isn't a problem today, but the flag is unstable, so > the default could change in a future compiler release. > > Link: https://github.com/rust-lang/rust/issues/121970 [1] > Link: https://github.com/rust-lang/rust/issues/121972 [2] > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > This patch raises the question of whether we should change the Rust > aarch64 support to use a custom target.json specification. If we do > that, then we can fix both the warning for dynamic SCS and the > build-failure for non-dynamic SCS without waiting for a new version of > rustc with the mentioned issues fixed. > --- > Changes in v2: > - Add -Cforce-unwind-tables flag. > - Link to v1: https://lore.kernel.org/r/20240304-shadow-call-stack-v1-1-f055eaf40a2c@google.com > --- > > Makefile | 1 + > arch/arm64/Makefile | 4 ++++ > 2 files changed, 5 insertions(+) > > diff --git a/Makefile b/Makefile > index 0e36eff14608..345066643a76 100644 > --- a/Makefile > +++ b/Makefile > @@ -936,6 +936,7 @@ ifdef CONFIG_SHADOW_CALL_STACK > ifndef CONFIG_DYNAMIC_SCS > CC_FLAGS_SCS := -fsanitize=shadow-call-stack > KBUILD_CFLAGS += $(CC_FLAGS_SCS) > +KBUILD_RUSTFLAGS += -Zsanitizer=shadow-call-stack > endif > export CC_FLAGS_SCS > endif > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile > index a88cdf910687..9bd5522c18e9 100644 > --- a/arch/arm64/Makefile > +++ b/arch/arm64/Makefile > @@ -48,9 +48,12 @@ KBUILD_AFLAGS += $(call cc-option,-mabi=lp64) > ifneq ($(CONFIG_UNWIND_TABLES),y) > KBUILD_CFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables > KBUILD_AFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables > +KBUILD_RUSTFLAGS += -Cforce-unwind-tables=n > else > KBUILD_CFLAGS += -fasynchronous-unwind-tables > KBUILD_AFLAGS += -fasynchronous-unwind-tables > +# TODO: Pass -Zuse-sync-unwind=n once we upgrade to Rust 1.77.0 > +KBUILD_RUSTFLAGS += -Cforce-unwind-tables=y > endif > That's the setup I used for my previous testing at [1], offering: Tested-by: Valentin Obst <kernel@valentinobst.de> Reviewed-by: Valentin Obst <kernel@valentinobst.de> - Best Valentin Link: https://lore.kernel.org/all/20240305112017.125061-1-kernel@valentinobst.de/ [1] > ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y) > @@ -103,6 +106,7 @@ endif > > ifeq ($(CONFIG_SHADOW_CALL_STACK), y) > KBUILD_CFLAGS += -ffixed-x18 > +KBUILD_RUSTFLAGS += -Ctarget-feature=+reserve-x18 > endif > > ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
On Tue, Mar 05, 2024 at 11:58:45AM +0000, Alice Ryhl wrote: > Add flags to support the shadow call stack sanitizer, both in the > dynamic and non-dynamic modes. > > Right now, the compiler will emit the warning "unknown feature specified > for `-Ctarget-feature`: `reserve-x18`". However, the compiler still > passes it to the codegen backend, so the flag will work just fine. Once > rustc starts recognizing the flag (or provides another way to enable the > feature), it will stop emitting this warning. See [1] for the relevant > issue. > > Currently, the compiler thinks that the aarch64-unknown-none target > doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if > you enable shadow call stack in non-dynamic mode. However, I still think > it is reasonable to add the flag now, as it will at least fail the build > when using an invalid configuration, until the Rust compiler is fixed to > list -Zsanitizer=shadow-call-stack as supported for the target. See [2] > for the feature request to add this. > > I have tested this change with Rust Binder on an Android device using > CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the > phone crashes immediately on boot, and with the flag, the phone appears > to work normally. > > This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag > defaults to n, so it isn't a problem today, but the flag is unstable, so > the default could change in a future compiler release. > > Link: https://github.com/rust-lang/rust/issues/121970 [1] > Link: https://github.com/rust-lang/rust/issues/121972 [2] > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > This patch raises the question of whether we should change the Rust > aarch64 support to use a custom target.json specification. If we do > that, then we can fix both the warning for dynamic SCS and the > build-failure for non-dynamic SCS without waiting for a new version of > rustc with the mentioned issues fixed. > --- > Changes in v2: > - Add -Cforce-unwind-tables flag. > - Link to v1: https://lore.kernel.org/r/20240304-shadow-call-stack-v1-1-f055eaf40a2c@google.com Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Sami
On Tue, Mar 05, 2024 at 01:14:19PM +0100, Miguel Ojeda wrote: > On Tue, Mar 5, 2024 at 12:58 PM Alice Ryhl <aliceryhl@google.com> wrote: > > > > Add flags to support the shadow call stack sanitizer, both in the > > dynamic and non-dynamic modes. > > > > Right now, the compiler will emit the warning "unknown feature specified > > for `-Ctarget-feature`: `reserve-x18`". However, the compiler still > > passes it to the codegen backend, so the flag will work just fine. Once > > rustc starts recognizing the flag (or provides another way to enable the > > feature), it will stop emitting this warning. See [1] for the relevant > > issue. > > > > Currently, the compiler thinks that the aarch64-unknown-none target > > doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if > > you enable shadow call stack in non-dynamic mode. However, I still think > > it is reasonable to add the flag now, as it will at least fail the build > > when using an invalid configuration, until the Rust compiler is fixed to > > list -Zsanitizer=shadow-call-stack as supported for the target. See [2] > > for the feature request to add this. > > > > I have tested this change with Rust Binder on an Android device using > > CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the > > phone crashes immediately on boot, and with the flag, the phone appears > > to work normally. > > > > This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag > > defaults to n, so it isn't a problem today, but the flag is unstable, so > > the default could change in a future compiler release. > > > > Link: https://github.com/rust-lang/rust/issues/121970 [1] > > Link: https://github.com/rust-lang/rust/issues/121972 [2] > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > --- > > This patch raises the question of whether we should change the Rust > > aarch64 support to use a custom target.json specification. If we do > > that, then we can fix both the warning for dynamic SCS and the > > build-failure for non-dynamic SCS without waiting for a new version of > > rustc with the mentioned issues fixed. > > If the arm64 maintainers are OK with the warning being triggered in that case: Sorry, I meant to reply on this at the time... > Acked-by: Miguel Ojeda <ojeda@kernel.org> > > Otherwise partially reverting to the `target.json` approach sounds good too. > > I added the `-Zuse-sync-unwind=n` to the list at > https://github.com/Rust-for-Linux/linux/issues/2. Given the default is > what we want, I have put it in the "Good to have" section. I think we have time to do this properly, like we did for the clang enablement a few years ago. In hindsight, avoiding hacks for the early toolchains back then was a really good idea because it meant we could rely on a solid baseline set of compiler features from the start. So, please can we fix this in rustc and just have SCS dependent on that? Cheers, Will
On Tue, Apr 9, 2024 at 12:31 PM Will Deacon <will@kernel.org> wrote: > > I think we have time to do this properly, like we did for the clang > enablement a few years ago. In hindsight, avoiding hacks for the early > toolchains back then was a really good idea because it meant we could > rely on a solid baseline set of compiler features from the start. Yeah, it sounds fair, thanks! After the warning is fixed (i.e. `-Zfixed-x18` or similar is implemented etc.), I would recommend adding support to the kernel for the `-Z` (unstable) flags, similar to this patch, in order to test them easily and getting `rustc` to stabilize them. Then the only change required should be a name change to `-C` or similar. Cheers, Miguel
Will Deacon <will@kernel.org> wrote: > On Tue, Mar 05, 2024 at 01:14:19PM +0100, Miguel Ojeda wrote: >> Otherwise partially reverting to the `target.json` approach sounds good too. >> >> I added the `-Zuse-sync-unwind=n` to the list at >> https://github.com/Rust-for-Linux/linux/issues/2. Given the default is >> what we want, I have put it in the "Good to have" section. > > I think we have time to do this properly, like we did for the clang > enablement a few years ago. In hindsight, avoiding hacks for the early > toolchains back then was a really good idea because it meant we could > rely on a solid baseline set of compiler features from the start. > > So, please can we fix this in rustc and just have SCS dependent on that? Hi Will, Just to keep you in the loop, I've posted a PR to make rustc recognize the reserve-x18 target feature, so that the -Ctarget-feature=+reserve-x18 flag stops emitting a warning. This should be sufficient for adding support for CONFIG_DYNAMIC_SCS. You can find it here: https://github.com/rust-lang/rust/pull/124323 As for non-dynamic SCS, I plan to tackle that after the PR is merged. See the "Future possibilities" section in the linked PR for more info on that. Alice
On Tue, Apr 30, 2024 at 11:09:25AM +0000, Alice Ryhl wrote: > Will Deacon <will@kernel.org> wrote: > > On Tue, Mar 05, 2024 at 01:14:19PM +0100, Miguel Ojeda wrote: > >> Otherwise partially reverting to the `target.json` approach sounds good too. > >> > >> I added the `-Zuse-sync-unwind=n` to the list at > >> https://github.com/Rust-for-Linux/linux/issues/2. Given the default is > >> what we want, I have put it in the "Good to have" section. > > > > I think we have time to do this properly, like we did for the clang > > enablement a few years ago. In hindsight, avoiding hacks for the early > > toolchains back then was a really good idea because it meant we could > > rely on a solid baseline set of compiler features from the start. > > > > So, please can we fix this in rustc and just have SCS dependent on that? > > Just to keep you in the loop, I've posted a PR to make rustc recognize > the reserve-x18 target feature, so that the -Ctarget-feature=+reserve-x18 > flag stops emitting a warning. > > This should be sufficient for adding support for CONFIG_DYNAMIC_SCS. > > You can find it here: > https://github.com/rust-lang/rust/pull/124323 > > As for non-dynamic SCS, I plan to tackle that after the PR is merged. > See the "Future possibilities" section in the linked PR for more info on > that. Thanks for persevering with this, Alice. I read the pull request above, but it looks like you went with: https://github.com/rust-lang/rust/pull/124655 instead, which was merged (hurrah!). Do we need anything else? Will
On Tue, Jun 4, 2024 at 4:29 PM Will Deacon <will@kernel.org> wrote: > > On Tue, Apr 30, 2024 at 11:09:25AM +0000, Alice Ryhl wrote: > > Will Deacon <will@kernel.org> wrote: > > > On Tue, Mar 05, 2024 at 01:14:19PM +0100, Miguel Ojeda wrote: > > >> Otherwise partially reverting to the `target.json` approach sounds good too. > > >> > > >> I added the `-Zuse-sync-unwind=n` to the list at > > >> https://github.com/Rust-for-Linux/linux/issues/2. Given the default is > > >> what we want, I have put it in the "Good to have" section. > > > > > > I think we have time to do this properly, like we did for the clang > > > enablement a few years ago. In hindsight, avoiding hacks for the early > > > toolchains back then was a really good idea because it meant we could > > > rely on a solid baseline set of compiler features from the start. > > > > > > So, please can we fix this in rustc and just have SCS dependent on that? > > > > Just to keep you in the loop, I've posted a PR to make rustc recognize > > the reserve-x18 target feature, so that the -Ctarget-feature=+reserve-x18 > > flag stops emitting a warning. > > > > This should be sufficient for adding support for CONFIG_DYNAMIC_SCS. > > > > You can find it here: > > https://github.com/rust-lang/rust/pull/124323 > > > > As for non-dynamic SCS, I plan to tackle that after the PR is merged. > > See the "Future possibilities" section in the linked PR for more info on > > that. > > Thanks for persevering with this, Alice. I read the pull request above, > but it looks like you went with: > > https://github.com/rust-lang/rust/pull/124655 > > instead, which was merged (hurrah!). Do we need anything else? Yeah, it took a while, but I've managed to get a -Zfixed-x18 flag in. It will be available starting with Rust 1.80, which will be released on the 25th of July. A few things: 1. The -Zsanitizer=shadow-call-stack flag still doesn't work because the compiler thinks that the target doesn't support it. I'll fix this eventually, but at least CONFIG_DYNAMIC_SCS works now. 2. I haven't convinced the Rust maintainers that -Zfixed-x18 is the way to go long term (flags starting with -Z are unstable and may change). Some of the maintainers want to instead add a x18-available target feature (that is, the inverse of the current reserve-x18 target feature), that you can disable with -Ctarget-feature=-x18-available. And a few questions for you: By the time support for 1.80 goes in, we are probably supporting more than one Rust compiler. For pre-1.80 compilers, should we fall back to -Ctarget-feature=+reserve-x18 (which emits a warning, but works), or fail compilation? Similarly, we should probably submit a fix to the stable branches so that SCS+Rust doesn't silently break in a hard-to-debug way. Do you prefer a backport with -Ctarget-feature=+reserve-x18 or one that fails compilation? Alice
Hi Alice, On Tue, Jun 04, 2024 at 04:53:16PM +0200, Alice Ryhl wrote: > On Tue, Jun 4, 2024 at 4:29 PM Will Deacon <will@kernel.org> wrote: > > On Tue, Apr 30, 2024 at 11:09:25AM +0000, Alice Ryhl wrote: > > > Will Deacon <will@kernel.org> wrote: > > > > On Tue, Mar 05, 2024 at 01:14:19PM +0100, Miguel Ojeda wrote: > > > >> Otherwise partially reverting to the `target.json` approach sounds good too. > > > >> > > > >> I added the `-Zuse-sync-unwind=n` to the list at > > > >> https://github.com/Rust-for-Linux/linux/issues/2. Given the default is > > > >> what we want, I have put it in the "Good to have" section. > > > > > > > > I think we have time to do this properly, like we did for the clang > > > > enablement a few years ago. In hindsight, avoiding hacks for the early > > > > toolchains back then was a really good idea because it meant we could > > > > rely on a solid baseline set of compiler features from the start. > > > > > > > > So, please can we fix this in rustc and just have SCS dependent on that? > > > > > > Just to keep you in the loop, I've posted a PR to make rustc recognize > > > the reserve-x18 target feature, so that the -Ctarget-feature=+reserve-x18 > > > flag stops emitting a warning. > > > > > > This should be sufficient for adding support for CONFIG_DYNAMIC_SCS. > > > > > > You can find it here: > > > https://github.com/rust-lang/rust/pull/124323 > > > > > > As for non-dynamic SCS, I plan to tackle that after the PR is merged. > > > See the "Future possibilities" section in the linked PR for more info on > > > that. > > > > Thanks for persevering with this, Alice. I read the pull request above, > > but it looks like you went with: > > > > https://github.com/rust-lang/rust/pull/124655 > > > > instead, which was merged (hurrah!). Do we need anything else? > > Yeah, it took a while, but I've managed to get a -Zfixed-x18 flag in. > It will be available starting with Rust 1.80, which will be released > on the 25th of July. Great, thank you! > A few things: > > 1. The -Zsanitizer=shadow-call-stack flag still doesn't work because > the compiler thinks that the target doesn't support it. I'll fix this > eventually, but at least CONFIG_DYNAMIC_SCS works now. > > 2. I haven't convinced the Rust maintainers that -Zfixed-x18 is the > way to go long term (flags starting with -Z are unstable and may > change). Some of the maintainers want to instead add a x18-available > target feature (that is, the inverse of the current reserve-x18 target > feature), that you can disable with -Ctarget-feature=-x18-available. > > And a few questions for you: > > By the time support for 1.80 goes in, we are probably supporting more > than one Rust compiler. For pre-1.80 compilers, should we fall back to > -Ctarget-feature=+reserve-x18 (which emits a warning, but works), or > fail compilation? I think we should just prevent the Kconfig option from being enabled if the toolchain doesn't give us what we need. So there's no need to fail compilation, but SCS =n. See how e.g. CONFIG_ARM64_BTI_KERNEL depends on CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI. > Similarly, we should probably submit a fix to the stable branches so > that SCS+Rust doesn't silently break in a hard-to-debug way. Do you > prefer a backport with -Ctarget-feature=+reserve-x18 or one that fails > compilation? As above, I think we should disable the feature in those cases. Make sense? Will
diff --git a/Makefile b/Makefile index 0e36eff14608..345066643a76 100644 --- a/Makefile +++ b/Makefile @@ -936,6 +936,7 @@ ifdef CONFIG_SHADOW_CALL_STACK ifndef CONFIG_DYNAMIC_SCS CC_FLAGS_SCS := -fsanitize=shadow-call-stack KBUILD_CFLAGS += $(CC_FLAGS_SCS) +KBUILD_RUSTFLAGS += -Zsanitizer=shadow-call-stack endif export CC_FLAGS_SCS endif diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index a88cdf910687..9bd5522c18e9 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -48,9 +48,12 @@ KBUILD_AFLAGS += $(call cc-option,-mabi=lp64) ifneq ($(CONFIG_UNWIND_TABLES),y) KBUILD_CFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables KBUILD_AFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables +KBUILD_RUSTFLAGS += -Cforce-unwind-tables=n else KBUILD_CFLAGS += -fasynchronous-unwind-tables KBUILD_AFLAGS += -fasynchronous-unwind-tables +# TODO: Pass -Zuse-sync-unwind=n once we upgrade to Rust 1.77.0 +KBUILD_RUSTFLAGS += -Cforce-unwind-tables=y endif ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y) @@ -103,6 +106,7 @@ endif ifeq ($(CONFIG_SHADOW_CALL_STACK), y) KBUILD_CFLAGS += -ffixed-x18 +KBUILD_RUSTFLAGS += -Ctarget-feature=+reserve-x18 endif ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
Add flags to support the shadow call stack sanitizer, both in the dynamic and non-dynamic modes. Right now, the compiler will emit the warning "unknown feature specified for `-Ctarget-feature`: `reserve-x18`". However, the compiler still passes it to the codegen backend, so the flag will work just fine. Once rustc starts recognizing the flag (or provides another way to enable the feature), it will stop emitting this warning. See [1] for the relevant issue. Currently, the compiler thinks that the aarch64-unknown-none target doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if you enable shadow call stack in non-dynamic mode. However, I still think it is reasonable to add the flag now, as it will at least fail the build when using an invalid configuration, until the Rust compiler is fixed to list -Zsanitizer=shadow-call-stack as supported for the target. See [2] for the feature request to add this. I have tested this change with Rust Binder on an Android device using CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the phone crashes immediately on boot, and with the flag, the phone appears to work normally. This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag defaults to n, so it isn't a problem today, but the flag is unstable, so the default could change in a future compiler release. Link: https://github.com/rust-lang/rust/issues/121970 [1] Link: https://github.com/rust-lang/rust/issues/121972 [2] Signed-off-by: Alice Ryhl <aliceryhl@google.com> --- This patch raises the question of whether we should change the Rust aarch64 support to use a custom target.json specification. If we do that, then we can fix both the warning for dynamic SCS and the build-failure for non-dynamic SCS without waiting for a new version of rustc with the mentioned issues fixed. --- Changes in v2: - Add -Cforce-unwind-tables flag. - Link to v1: https://lore.kernel.org/r/20240304-shadow-call-stack-v1-1-f055eaf40a2c@google.com --- Makefile | 1 + arch/arm64/Makefile | 4 ++++ 2 files changed, 5 insertions(+) --- base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72 change-id: 20240304-shadow-call-stack-9c197a4361d9 Best regards,