All of lore.kernel.org
 help / color / mirror / Atom feed
From: Valentin Obst <kernel@valentinobst.de>
To: samitolvanen@google.com, aliceryhl@google.com
Cc: Jamie.Cunliffe@arm.com, a.hindborg@samsung.com,
	alex.gaynor@gmail.com, ardb@kernel.org, benno.lossin@proton.me,
	bjorn3_gh@protonmail.com, boqun.feng@gmail.com,
	broonie@kernel.org, catalin.marinas@arm.com, gary@garyguo.net,
	keescook@chromium.org, linux-arm-kernel@lists.infradead.org,
	linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
	mark.rutland@arm.com, masahiroy@kernel.org, maz@kernel.org,
	nathan@kernel.org, ndesaulniers@google.com, nicolas@fjasle.eu,
	ojeda@kernel.org, rust-for-linux@vger.kernel.org,
	wedsonaf@gmail.com, will@kernel.org,
	Valentin Obst <kernel@valentinobst.de>
Subject: Re: [PATCH] rust: add flags for shadow call stack sanitizer
Date: Tue,  5 Mar 2024 00:31:51 +0100	[thread overview]
Message-ID: <20240304233151.248925-1-kernel@valentinobst.de> (raw)
In-Reply-To: <CABCJKuem3GbLO-G7+wi8LPA8rFgNzFVjNof7zcAO1UGJR4u44Q@mail.gmail.com>

> >
> > 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.
> >
> > 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>
> > ---
> > It's not 100% clear to me whether this patch is enough for full SCS
> > support in Rust. If there is some issue where this makes things compile
> > and work without actually applying SCS to the Rust code, please let me
> > know. Is there some way to verify that it is actually working?
>
> Perhaps you could write a Rust version of the CFI_BACKWARD test in LKDTM?
>
> Alternatively, the simplest way to verify this is to look at the
> disassembly and verify that shadow stack instructions are emitted to
> Rust functions too. In case of dynamic SCS, you might need to dump
> function memory in a debugger to verify that PAC instructions were
> patched correctly. If they're not, the code will just quietly continue
> working without using shadow stacks.

Was just in the process of doing that:

- `paciasp`/`autiasp` pairs are emitted for functions in Rust modules.
- Rust modules have no `.init.eh_frame` section, which implies that
  `module_finalize` is _not_ rewriting the pac insns when SCS is dynamic.
  - Confirmed that behavior in the debugger (C modules and the C part of the
    kernel are correctly rewritten, Rust modules execute with
    `paciasp`/`autiasp` still in place).
- Kernel boots just fine with Rust kunit tests, tested with and without dynamic
  SCS, i.e., on a CPU that supports PAC/BTI and one that does not.
- Rust sample modules load and unload without problems as well.
- `x18` is indeed not used in the codegen.

I guess we might be able to get this working when we tweak the build system
to emit the missing section for Rust modules.

    - Best Valentin

>
> > 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.
>
> Sure, having a custom target description for the kernel might be
> useful for other purposes too. In the meantime:
>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
>
> Sami
>
>

WARNING: multiple messages have this Message-ID (diff)
From: Valentin Obst <kernel@valentinobst.de>
To: samitolvanen@google.com, aliceryhl@google.com
Cc: Jamie.Cunliffe@arm.com, a.hindborg@samsung.com,
	alex.gaynor@gmail.com, ardb@kernel.org, benno.lossin@proton.me,
	bjorn3_gh@protonmail.com, boqun.feng@gmail.com,
	broonie@kernel.org, catalin.marinas@arm.com, gary@garyguo.net,
	keescook@chromium.org, linux-arm-kernel@lists.infradead.org,
	linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
	mark.rutland@arm.com, masahiroy@kernel.org, maz@kernel.org,
	nathan@kernel.org, ndesaulniers@google.com, nicolas@fjasle.eu,
	ojeda@kernel.org, rust-for-linux@vger.kernel.org,
	wedsonaf@gmail.com, will@kernel.org,
	Valentin Obst <kernel@valentinobst.de>
Subject: Re: [PATCH] rust: add flags for shadow call stack sanitizer
Date: Tue,  5 Mar 2024 00:31:51 +0100	[thread overview]
Message-ID: <20240304233151.248925-1-kernel@valentinobst.de> (raw)
In-Reply-To: <CABCJKuem3GbLO-G7+wi8LPA8rFgNzFVjNof7zcAO1UGJR4u44Q@mail.gmail.com>

> >
> > 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.
> >
> > 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>
> > ---
> > It's not 100% clear to me whether this patch is enough for full SCS
> > support in Rust. If there is some issue where this makes things compile
> > and work without actually applying SCS to the Rust code, please let me
> > know. Is there some way to verify that it is actually working?
>
> Perhaps you could write a Rust version of the CFI_BACKWARD test in LKDTM?
>
> Alternatively, the simplest way to verify this is to look at the
> disassembly and verify that shadow stack instructions are emitted to
> Rust functions too. In case of dynamic SCS, you might need to dump
> function memory in a debugger to verify that PAC instructions were
> patched correctly. If they're not, the code will just quietly continue
> working without using shadow stacks.

Was just in the process of doing that:

- `paciasp`/`autiasp` pairs are emitted for functions in Rust modules.
- Rust modules have no `.init.eh_frame` section, which implies that
  `module_finalize` is _not_ rewriting the pac insns when SCS is dynamic.
  - Confirmed that behavior in the debugger (C modules and the C part of the
    kernel are correctly rewritten, Rust modules execute with
    `paciasp`/`autiasp` still in place).
- Kernel boots just fine with Rust kunit tests, tested with and without dynamic
  SCS, i.e., on a CPU that supports PAC/BTI and one that does not.
- Rust sample modules load and unload without problems as well.
- `x18` is indeed not used in the codegen.

I guess we might be able to get this working when we tweak the build system
to emit the missing section for Rust modules.

    - Best Valentin

>
> > 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.
>
> Sure, having a custom target description for the kernel might be
> useful for other purposes too. In the meantime:
>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
>
> Sami
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2024-03-04 23:39 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-04 13:17 [PATCH] rust: add flags for shadow call stack sanitizer Alice Ryhl
2024-03-04 13:17 ` Alice Ryhl
2024-03-04 20:09 ` Sami Tolvanen
2024-03-04 20:09   ` Sami Tolvanen
2024-03-04 23:31   ` Valentin Obst [this message]
2024-03-04 23:31     ` Valentin Obst
2024-03-05  7:15     ` Alice Ryhl
2024-03-05  7:15       ` Alice Ryhl
2024-03-05 11:20       ` Valentin Obst
2024-03-05 11:20         ` Valentin Obst
2024-03-05 11:28         ` Alice Ryhl
2024-03-05 11:28           ` Alice Ryhl
2024-03-05 11:31           ` Alice Ryhl
2024-03-05 11:31             ` Alice Ryhl
2024-03-05 12:08             ` Alice Ryhl
2024-03-05 12:08               ` Alice Ryhl
2024-03-05 12:09   ` Miguel Ojeda
2024-03-05 12:09     ` Miguel Ojeda

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240304233151.248925-1-kernel@valentinobst.de \
    --to=kernel@valentinobst.de \
    --cc=Jamie.Cunliffe@arm.com \
    --cc=a.hindborg@samsung.com \
    --cc=alex.gaynor@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=ardb@kernel.org \
    --cc=benno.lossin@proton.me \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=gary@garyguo.net \
    --cc=keescook@chromium.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=masahiroy@kernel.org \
    --cc=maz@kernel.org \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=nicolas@fjasle.eu \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=samitolvanen@google.com \
    --cc=wedsonaf@gmail.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.