Emit endbr64 instructions on amd64 to support OpenBSD indirect branch target control flow enforcement. #13023
Conversation
OpenBSD added support for indirect branch target control flow enforcement in 7.4. Previously, ocaml used a linker argument to disable enforcement on OpenBSD. This commit inserts endbr64 instructions after function declarations and labels. Disabling the use of the linker argument is committed separately.
The native-code compiler does not support i386 anymore, so nothing needs to be done. |
- remove some extraneous endbr64 usage around some globals - remove endbr64 for cfi_startproc, move to function/funcdecl
|
Also, there is the risk that, on some targets (such as macos...), |
Hmm good point. Working on a tweak to do this. Is it safe to say if the system isn't macOS or Windows, skip emitting endbr64? Or should I overcome my fear of autoconf and build in an actual check that the assembler available can handle an |
|
Dave Voutila (2024/03/11 10:40 -0700):
> Also, there is the risk that, on some targets (such as macos...), `as` does not recognize `endbr64`, so the use of this instruction should be made conditional (at least target system-dependent) in `asmcomp` and `runtime`...
Hmm good point. Working on a tweak to do this. Is it safe to say if
the system isn't macOS or Windows, skip emitting endbr64? Or should I
overcome my fear of autoconf and build in an actual check that the
assembler available can handle an `endbr64` instruction 馃槵
Feature tests are preferred.
You may want to take inspiration from the
`OCAML_AS_HAS_CFI_DIRECTIVES`macro defined in `aclocal.m4`, from line
209. Of course you willhave to get this macro called from
`configure.ac`, as `OCAML_AS_HAS_CFI_DIRECTIVES` is. Once you have
modified `ocnfigure.ac`, run `tools/autogen` to regenerate `configure`,
whosemodifications need to be commited alongside those of the files that
caused them.
|
Unfortunately, given there are about |
|
I dusted off the autotools portion of my brain. Got something working that tests an |
|
Dave Voutila (2024/03/11 16:11 -0700):
I dusted off the autotools portion of my brain. Got something working
that tests an `endbr64` instruction. Not sure if it's completely valid
as it's not using the local assembler directly. (For some reason I
cannot for the life of me get that to work as I'm not getting shell
expansion of something like `$AS`.)
Sorry, why didn't you take inspiration from the existing macro, as I suggested? Wouldn't that have avoided the problem you describe?
|
|
@shindere ah, I missed your message. Yes that looks like the approach I want for the assembler test. I'll adapt my latest changes. |
|
@shindere My latest commit creates and uses an m4 macro. For the assembler, do I need to check the preprocessor assembler, too? |
|
This is starting to take shape. In order to help cross-platform maintainability, I suggest the following instead:
|
|
Dave Voutila (2024/03/12 12:02 -0700):
@shindere My latest commit creates and uses an m4 macro. For the
assembler, do I need to check the preprocessor assembler, too?
I don't think so. I mean, if that check turns out to be useful then we
will have to make sure every macro that does some assembler test will
have to test both assemblers, but given this is not the case for the
test mentionned above I don't think things need to be different for your
test, especially given that it does not seem to rely on C preprocessor
macros specifically.
What do others think about also testing with `ASPP` when we do assembler
tests? @xavierleroy? @nojb? Others?
|
|
The newer approach of There are some cases of extra |
You can probably drop the |
|
Dave Voutila (2024/03/13 17:43 -0700):
There are some cases of extra `endbr64` instructions being emitted.
Might need some eyeballs from someone more familiar with
`asmcomp/amd64/emit.mlp`.
I'll wait a general agreement on the PR itself to review the configure
details.
|
Unfortunately, CET affects any indirect |
|
The impact of this change on code size and on execution speed needs to be assessed. Until it is proved to be completely negligible, I would prefer a configure flag to select endbr64 instruction generation. This flag would be on by default on OpenBSD 7.4 and other platforms that mandate control-flow integrity checking, and off by default on all other platforms. |
|
Xavier Leroy (2024/03/15 08:09 -0700):
The impact of this change on code size and on execution speed needs to be assessed.
Until it is proved to be completely negligible, I would prefer a
configure flag to select endbr64 instruction generation. This flag
would be on by default on OpenBSD 7.4 and other platforms that mandate
control-flow integrity checking, and off by default on all other
platforms.
If configure adjustments are needed I can definitely help.
|
I've just updated to tuck the detection and enablement behind a |
|
Am I correct that there are platforms were this is a requirement (i.e., if it's not enabled things simply don't work)? If that's correct, how about enabling it only when it turns out it's necessary, at least for the time being?
|
In the current state of things (without this PR), extra linker flags are used to ask for no call-flow-integrity enforcement. The plan in this PR is to emit the needed |
|
Thinking through putting this behind a feature flag. How does opam gain new options for usage with |
|
Thanks for putting this patch together @voutilad; perfect timing to fix OCaml on my desktop! I've confirmed it passes all tests on my OpenBSD-current/x86_64 box which has hardware cfbti support (and that OCaml segfaults with SIGILLs without this patch). I've pushed two patches to avsm@bd46cbf and avsm@5ac8012 that you may want to pull into your branch. They change the configure behaviour to that suggested by @xavierleroy above:
There's about a 1.5%-2% code size increase with the endbr64 instructions emitted for ocamlopt.opt, so there's probably a performance hit here. However, I noticed that we're sometimes emitting double endbr64s for some OCaml function preludes; e.g. from ocamlopt.opt below. Is this intentional? edit: sorry, I missed the discussion of this duplication above with @dustanddreams! It's tempting to use NOTRACK isn't it ;-) |
This also obeys an explicit directive for the configure flag if specified. So --enable-bti will always attempt to compile with BTI, and --disable-bti will always disable it (even if required on the platform). Not specifying the flag will cause the configure script to try and do the right thing.
Depending on the presence of cet.h will end up emitting them on most modern compilers, so this specifically checks for the --enable-bti flag being pass to configure
@avsm no, definitely not intentional. I noticed that as well and haven't been able to find the exact cause, but I'm thinking it's how functions and labels are emitted in OCaml. Since |
|
@avsm cherry-picked those commits, btw, and regenerated |
|
This is looking good. However, if one explicitly runs |
|
As a naive non-configure expert, I would expect a hard failure at configure time if the user asks for |
|
I confirm: if a feature is explictly requested and it can be determined
by testing that the requested feature won't be available, this should be
reported as an error.
|
|
Miod Vallat (2024/04/03 00:00 -0700):
This is looking good. However, if one explicitly runs `configure
--enable-bti`, you should nevertheless check for `cet.h` and only
enable (`bti=true`) if found. Otherwise, naive ~~ricers~~ gentoo users
will try to use this configure switch on systems lacking cfi support.
Confirmed. If the feature is explicitly disabled then no test should
happen. Otherwise the test should be done and result either in the
feature being disabled (if it had not been enabled) or in an error (if
it had been explictly enabled).
|
|
Checking for cet.h really seems unnecessary to me, since we also need to support arm64; does anyone have a machine with this activated? I tried to get OpenBSD/arm64 bti support to kick in by booting a qemu/hvt VM on my M2 Mac, but it didn't pass through enough ARMv8 instruction goodness to activate the enforcement... |
Checking for There is no similar file for arm (yet). I don't have access to arm64 systems with working control flow enforcement. Try installing a native OpenBSD on your M2 on an external USB disk (-: |
|
FWIW, my arm64 devices that run OpenBSD also don't support BTI 馃槥 . I'm looking to get one soon, but at the moment have no way to work on that support as part of this PR. |
|
Well, I now have an OpenBSD arm64 machine with BTI...so if we can agree on a general path forward here for amd64 I can also draft a PR for arm64. |
Actually I think that BTI support will be active even if qemu doesn't pass through the processor feature bit. At least someone reported that this happens on an M3 Mac. I haven't decided whether I consider this a bug or a feature ;) |
asmcomp/amd64/emit.mlp
Outdated
| @@ -897,6 +897,7 @@ let fundecl fundecl = | |||
| D.global (emit_symbol fundecl.fun_name); | |||
| D.label (emit_symbol fundecl.fun_name); | |||
| emit_debug_info fundecl.fun_dbg; | |||
| I.endbr64 (); | |||
There was a problem hiding this comment.
The endbr64 instruction should really be after .cfi_startproc
|
@dustanddreams, @avsm how do you want me to proceed with the PR? |
|
Well I still would like the existence of |
I made some measurements on x86_64, see the table below. For ocamlopt.opt there's a 2.9% size increase for the code segment (as reported by So, I'm going to insist that endbr64 instructions should be generated only for labels that can be the targets of indirect jumps. If I'm not mistaken, these are the labels mentioned in |
gasche
added
back-end
enhancement
submitter-action-needed
A previous PR (#12918) set the linker arg
nobtcfiwhen building on OpenBSD. This PR adds in usage of theendbr64instruction to allow dropping that linker arg and supporting this type of control flow enforcement on OpenBSD and any other platforms that adopt it. A separate commit in the PR drops the linker arg.I've never hacked on the ocaml compiler before, so not sure if work needs to be done to tweak for
i386. Currently, OpenBSD doesn't support indirect branch target enforcement oni386and I'm not aware of any dev currently working on it. (If it was in use, we'd need anendbr32instruction instead.)I've only been able to test on
amd64, but the resulting native binaries run and tests seem to pass:Couple points to note:
endbr64usage I'm proposing to find the minimal change needed. Some of the insertions at labels may not be required, but it's hard to tell without experimentation. (At least from my very limited knowledge of the compiler.)aarch64for OpenBSD. Currently onlyamd64andarm64platforms enforce IBT on OpenBSD. If and when support forriscv64, the pattern will need to be changed...but I'm willing to get ocaml will get the necessary tweaks to drop this linker flag forarm64before then 馃槅I've tested a port of this change to 5.1 and 4.14. The changes are similar, but need some slight tweaking.