Received-SPF: pass (google.com: domain of linux-kernel+bounces-92280-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <CAH5fLgg0yGbuHnMbMB103Zssg4KSfXUR3kvhr0kuqTSah=6kWg@mail.gmail.com>
 <20240305112017.125061-1-kernel@valentinobst.de> <CAH5fLgis7Usg_cfWM5rBKjRKsB4857PkyuCMrCU6PmEqTOMFiw@mail.gmail.com>
In-Reply-To: <CAH5fLgis7Usg_cfWM5rBKjRKsB4857PkyuCMrCU6PmEqTOMFiw@mail.gmail.com>
From: Alice Ryhl <aliceryhl@google.com>
Date: Tue, 5 Mar 2024 12:31:22 +0100
Message-ID: <CAH5fLgj=iuWNWy9jGvaD0SCcZg6GSj9XBgB0vZwL8EokobUPMg@mail.gmail.com>
Subject: Re: [PATCH] rust: add flags for shadow call stack sanitizer
To: Valentin Obst <kernel@valentinobst.de>
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, samitolvanen@google.com, wedsonaf@gmail.com, 
	will@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 5, 2024 at 12:28=E2=80=AFPM Alice Ryhl <aliceryhl@google.com> w=
rote:
>
> On Tue, Mar 5, 2024 at 12:20=E2=80=AFPM Valentin Obst <kernel@valentinobs=
t.de> wrote:
> >
> > > >>> It's not 100% clear to me whether this patch is enough for full S=
CS
> > > >>> support in Rust. If there is some issue where this makes things c=
ompile
> > > >>> and work without actually applying SCS to the Rust code, please l=
et 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 cont=
inue
> > > >> working without using shadow stacks.
> > > >
> > > > Was just in the process of doing that:
> > > >
> > > > - `paciasp`/`autiasp` pairs are emitted for functions in Rust modul=
es.
> > > > - Rust modules have no `.init.eh_frame` section, which implies that
> > > >   `module_finalize` is _not_ rewriting the pac insns when SCS is dy=
namic.
> > > >   - Confirmed that behavior in the debugger (C modules and the C pa=
rt 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 wit=
hout 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 buil=
d system
> > > > to emit the missing section for Rust modules.
> > >
> > > I suppose the -Cforce-unwind-tables=3Dy flag will most likely do it.
> >
> > Yes, enabling this means that `.eh_frame` sections, which are converted=
 to
> > `.init.eh_frame` sections for loadable modules, are generated for Rust
> > objects.
> >
> > Tested booting, kunit tests, sample modules (as builtin and loadable) f=
or
> > both, dynamic SCS active and inactive. Backtraces on Rust panicks also =
look
> > normal.
> >
> > Confirmed that in the debugger that builtin and external modules are
> > rewritten (or not rewritten if no dynamic SCS). Did not check that the
> > `eh_frame` sections are exhaustive, i.e., cover all `paciasp`/`autiasp`
> > pairs, only verified a few functions (in init text and normal text).
>
> Thank you for checking that!
>
> > > There's also an use_sync_unwind option, but it defaults to no, so it
> > > doesn't seem like we need to set it.
> >
> > Are those defaults stable or will we notice if they change? If not it m=
ight
> > make sense to set it explicitly anyways to avoid surprises in the futur=
e.
>
> The flag itself is unstable, so I imagine that nothing is promised about =
it.
>
> I tried it, but I wasn't actually able to find a way to set it. I can
> see the flag in the rustc source code, but passing -Zuse-sync-unwind=3Dn
> results in "error: unknown unstable option: `use-sync-unwind`". Not
> sure what the issue is.

Oh, I understand now. It's really recent and not in 1.73.0, which is
what I'm using in the Android build.

Alice