build: Mark `x86_64-linux-gnu` release binaries as CET-enabled #30685

hebasto · 2024-08-20T17:39:48Z

CET is Intel’s Control-flow Enforcement Technology.

The current GCC implementation of the -fcf-protection option is based on CET for x86_64-linux-gnu.

However, on the master branch @ d79ea80, the release binaries are not marked as CET-enabled:

$ env HOSTS=x86_64-linux-gnu ./contrib/guix/guix-build
$ tar -xf guix-build-d79ea809d281/output/x86_64-linux-gnu/bitcoin-d79ea809d281-x86_64-linux-gnu.tar.gz
$ readelf -n bitcoin-d79ea809d281/bin/bitcoind | grep -A 5 "\.note\.gnu\.property"
Displaying notes found in: .note.gnu.property
  Owner                Data size 	Description
  GNU                  0x00000020	NT_GNU_PROPERTY_TYPE_0
      Properties: x86 feature used: x86, x87, XMM, YMM, XSAVE
	x86 ISA used: x86-64-baseline, x86-64-v2, x86-64-v3

This occurs because not all object files, including those from the depends and the secp256k1 subtree, have the required properties.

This PR resolves the issue by explicitly enabling -fcf-protection=full for all object files, which will be beneficial for all targets, not just x86_64-linux-gnu.

With this PR:

$ env HOSTS=x86_64-linux-gnu ./contrib/guix/guix-build
$ tar -xf guix-build-c5bed747e6e9/output/x86_64-linux-gnu/bitcoin-c5bed747e6e9-x86_64-linux-gnu.tar.gz
$ readelf -n bitcoin-c5bed747e6e9/bin/bitcoind | grep -A 6 "\.note\.gnu\.property"
Displaying notes found in: .note.gnu.property
  Owner                Data size 	Description
  GNU                  0x00000030	NT_GNU_PROPERTY_TYPE_0
      Properties: x86 feature: IBT, SHSTK
	x86 feature used: x86, x87, XMM, YMM, XSAVE
	x86 ISA used: x86-64-baseline, x86-64-v2, x86-64-v3

Please note Properties: x86 feature: IBT, SHSTK.

A runtime check on Ubuntu 24.04 (GLIBC 2.39):

$ export GLIBC_TUNABLES=glibc.cpu.hwcaps=SHSTK
$ bitcoin-c5bed747e6e9/bin/bitcoind -printtoconsole=0 &
$ cat /proc/$(cat .bitcoin/bitcoind.pid)/status | grep x86
x86_Thread_features:	shstk 
x86_Thread_features_locked:	shstk wrss

As a follow-up, a check for the IBT and SHSTK can be added to the contrib/devtools/security-check.py script.

Fixes #30677.

DrahtBot · 2024-08-20T17:39:51Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30712 (fuzz: Add missing fuzz targets to cmake build by maflcko)
#30465 (depends: Set CMAKE_SYSTEM_VERSION for CMake builds by hebasto)
#30454 (build: Introduce CMake-based build system by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake · 2024-08-20T17:52:08Z

Concept NACK on the approach here (CI is showing why it wont work with depends, I assume it will also break other builds (where we haven't built the libs)). I'd rather build up from from something like #30438 (and have a branch with similar changes).

As a follow-up, a check for the IBT and SHSTK can be added to the contrib/devtools/security-check.py script.

I don't think we should be making a change this invasive and deferring the addition of checks until later.

DrahtBot · 2024-08-20T18:37:07Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/29015496081}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

hebasto · 2024-08-21T09:48:09Z

I'd rather build up from from something like #30438 (and have a branch with similar changes).

Is that branch publicly available?

hebasto · 2024-08-21T19:49:16Z

Concept NACK on the approach here (CI is showing why it wont work with depends, I assume it will also break other builds (where we haven't built the libs)). I'd rather build up from from something like #30438 (and have a branch with similar changes).

CI is green.
This approach works and fixes #30677.
#30438 does not indicate that it is capable of resolving #30677.

fanquake · 2024-08-21T19:58:40Z

depends/hosts/linux.mk

 x86_64_linux_NM=nm
 x86_64_linux_STRIP=strip
+
+ifeq ($(NO_HARDEN),)


I don't think this is the right place for these flags. If we want them in the Guix build, then we should add them to the Guix build, not hardcode them into depends.

Can you elaborate on how you think we should in general decide where configuration should live (default in configure/cmake vs depends vs guix)?

My thinking would be that depends builds are intended to be as close to production binaries as we can get them, but without having a deterministic build environment (so without controlling compiler or dependency versions).

without controlling ... dependency versions

I might be misunderstanding you, but one of the main reasons depends exists is to control dependency versions? i.e as-opposed to using whatever ships with the system packages.

so without controlling compiler

This change is a good example of trying to add that control; i.e hardcoding compiler flags with no check for if they are actually supported/or work as expected.

Can you elaborate on how you think we should in general decide where configuration should live (default in configure/cmake vs depends vs guix)?

Sure! I'll follow up with something lengthier tomorrow.

I might be misunderstanding you, but one of the main reasons depends exists is to control dependency versions? i.e as-opposed to using whatever ships with the system packages.

Err, yes, of course. I had compiler-infused dependencies like glibc in mind, but that's the exception rather than the rule.

This change is a good example of trying to add that control; i.e hardcoding compiler flags with no check for if they are actually supported/or work as expected.

Ah that makes sense. Your concern here is that this may not work (or not work as expected) on the full range of environments where we expect depends-builds to work?

Your concern here is that this may not work (or not work as expected) on the full range of environments where we expect depends-builds to work?

Yea. It's also somewhat displayed by the inconsistency of the change here; C/XXFLAGS is being put into depends, but the LDFLAGS are put into Guix. The LDFLAGS can't be hard-coded into depends, because that'll break depends builds where the libc/other base libs haven't been compiled with support for CET (for example, the current release of Alpine Linux on x86_64), or, any other builds where we use other libs compiled without such support, fuzzing, sanitizers etc.

Doing so would also add a requirement on the version of the linker, and possibly which ones
you can use. For any given flag, ld,lld, mold etc may/or may not implement it, and
will do so at various times (same holds for GCC vs Clang and compilation flags).

The C/CXXFLAGS addition might currently be ok (at least, nothing we test in CI is broken), and we know these flags are atleast "supported" by our minimum GCC and Clang, but now we're just playing whackamole with where things end up, and have got the inconsistency of spreading the flags for a certain feature across the two different systems, because after enough shuffling it now compiles and (hopefully) doesn't break anything.

Other concerns:

Generally, this approach of hardcoding specific flags for specific targets (while simeltaneous just assuming they'll work) doesn't scale well: i.e for other x86_64 targets where these compilation flags may be relevant; are we just copy-pasting the same blocks of flags around inside depends? That's messy.

You've now got 3 different places to consider when thinking about hardening logic/flags and
their interactions (configure, depends, Guix), and, for anyone wanting to add new flags, it's even less
clear where they should live, or why.

Can you elaborate on how you think we should in general decide where configuration should live
(default in configure/CMake vs depends vs Guix)?

Ideally, depends should remain generic/assumption free, and then users (including us), can use their distro of choice (which, for example, may already have it's own hardened toolchain), or bring their own compiler /
flags, and things will pretty much always just "work". We can then try flags generically/best effort in configure/CMake, and anyone can always continue to override what we are doing.

In that sense, you can think of Guix as essentially Cores own distro, where we have full
control to choose the compiler, glibc, binutils etc, configure them exactly as (and as hardened as)
we want, and then use that to compile depends and Core, with whichever additional flags we prefer for release
builds (that have additional post-build checks and ideally some coverage in CI).

fanquake · 2024-08-22T10:10:42Z

contrib/guix/libexec/build.sh

 esac

+case "$HOST" in
+    x86_64-linux-gnu)  HARDENED_LDFLAGS="-Wl,-z,cet-report=error" ;;


I don't think splitting flags into LDFLAGS and HARDENED_LDFLAGS inside Guix makes sense, as, everything in Guix is meant to be a "hardened" build.

This is done to prevent disruption of ~~internal flag checks~~ test-security-check.py.

What disruption?

x86_64-linux-gnu-ld: /tmp/ccwqGS4u.o: error: missing IBT and SHSTK properties collect2: error: ld returned 1 exit status E ====================================================================== ERROR: test_ELF (__main__.TestSecurityChecks) ---------------------------------------------------------------------- Traceback (most recent call last): File "/distsrc-base/distsrc-5fda052191b0-x86_64-linux-gnu/./contrib/devtools/test-security-check.py", line 67, in test_ELF self.assertEqual(call_security_check(cxx, source, executable, pass_flags + ['-fcf-protection=none']), (1, executable + ': failed CONTROL_FLOW')) File "/distsrc-base/distsrc-5fda052191b0-x86_64-linux-gnu/./contrib/devtools/test-security-check.py", line 42, in call_security_check subprocess.run([*cxx,source,'-o',executable] + env_flags() + options, check=True) File "/gnu/store/fn4rrnx7vx1bl6mccrhm95y6br8yg9s6-python-minimal-3.10.7/lib/python3.10/subprocess.py", line 524, in run raise CalledProcessError(retcode, process.args,

GIven that the whole point of these scripts is to sanity check the (release) tests, using the release flags, and in the Guix env; I don't think an undocumented workaround, of just hiding certain release flags, from the test scripts, is the right thing to do.

OK. What can you suggest?

OK. What can you suggest?

My suggestion is to remove the changes that have been added to facilitate bypassing tests (i.e separating the LDFLAGS into LDFLAGS and HARDENED_LDFLAGS, so that some flags are skipped when tests are performed). If the build environment has changed such that the current set of tests don't pass with the new changes, then the tests should be reworked, not skipped.

fanquake · 2024-08-22T11:10:21Z

Guix building 89f8e8e on aarch64 doesn't work:

time HOSTS="x86_64-linux-gnu" ./contrib/guix/guix-build
<snip>
  CXXLD    test/test_bitcoin
x86_64-linux-gnu-ld: /bitcoin/depends/x86_64-linux-gnu/lib/libevent.a(buffer.c.o): error: missing IBT and SHSTK properties
x86_64-linux-gnu-ld: /bitcoin/depends/x86_64-linux-gnu/lib/libevent.a(bufferevent.c.o): error: missing IBT and SHSTK properties
<snip>
x86_64-linux-gnu-ld: /bitcoin/depends/x86_64-linux-gnu/lib/libzmq.a(mechanism_base.cpp.o): error: missing IBT and SHSTK properties
collect2: error: ld returned 1 exit status

hebasto · 2024-08-22T13:58:18Z

The PR description has been updated with a runtime check example.

hebasto · 2024-08-22T15:36:22Z

Guix building 89f8e8e on aarch64 doesn't work:

The issue should be fixed now.

DrahtBot · 2024-08-28T09:56:15Z

🐙 This pull request conflicts with the target branch and needs rebase.

fanquake · 2024-10-01T12:58:01Z

What is the status of this?

hebasto · 2024-10-11T19:03:06Z

Apparently, I haven't had time to address feedback and rebase this PR recently. Perhaps it could be labelled "Up for grabs"?

fanquake · 2024-10-16T14:05:23Z

Perhaps it could be labelled "Up for grabs"?

I'll cherry-pick / followup with this.

4d3da08 guix: Enable CET for `glibc` package (Hennadii Stepanov) Pull request description: Pulled from #30685. This doesn't need to wait for anything. ACKs for top commit: laanwj: ACK 4d3da08 TheCharlatan: ACK 4d3da08 Tree-SHA512: 1f4645971381fd342adec52c826fc0023722519a3e28043c9fe8b64bbc1abad822fcc25a64f3f959e3f3a10f5c119029f4cae13c22bac6badcbec9ae8b501dfc

hebasto added the Build system label Aug 20, 2024

DrahtBot added the CI failed label Aug 20, 2024

DrahtBot mentioned this pull request Aug 21, 2024

build: Remove Autotools-based build system #30664

Merged

hebasto marked this pull request as draft August 21, 2024 10:01

fanquake mentioned this pull request Aug 21, 2024

guix: (explicitly) build Linux GCC with --enable-cet #30438

Merged

hebasto force-pushed the 240820-control-flow branch from 615b94c to 89f8e8e Compare August 21, 2024 14:55

hebasto marked this pull request as ready for review August 21, 2024 14:56

DrahtBot removed the CI failed label Aug 21, 2024

fanquake reviewed Aug 21, 2024

View reviewed changes

DrahtBot mentioned this pull request Aug 22, 2024

build: Introduce CMake-based build system #30454

Merged

fanquake reviewed Aug 22, 2024

View reviewed changes

DrahtBot mentioned this pull request Aug 22, 2024

guix: Pointer Authentication and Branch Target Identification for aarch64 Linux #24123

Closed

hebasto added 3 commits August 22, 2024 14:28

depends: Enable control-flow protection x86 Linux targets

a8c08ef

guix: Enable CET for glibc package

636fba1

guix: Check for IBT and SHSTK properties in .note.gnu.property section

c5bed74

hebasto force-pushed the 240820-control-flow branch from 89f8e8e to c5bed74 Compare August 22, 2024 13:28

This was referenced Aug 22, 2024

depends: Set CMAKE_SYSTEM_VERSION for CMake builds #30465

Closed

fuzz: Add missing fuzz targets to cmake build #30712

Merged

DrahtBot added the Needs rebase label Aug 28, 2024

fanquake mentioned this pull request Oct 11, 2024

Consider making 27.x Long-Term Support (LTS) #31068

Closed

hebasto marked this pull request as draft October 11, 2024 19:03

fanquake closed this Oct 16, 2024

fanquake mentioned this pull request Oct 20, 2024

guix: Enable CET for glibc package #31121

Merged

bitcoin locked and limited conversation to collaborators Oct 16, 2025

build: Mark x86_64-linux-gnu release binaries as CET-enabled #30685

build: Mark x86_64-linux-gnu release binaries as CET-enabled #30685

Uh oh!

Conversation

hebasto commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

fanquake commented Aug 20, 2024

Uh oh!

DrahtBot commented Aug 20, 2024

Uh oh!

hebasto commented Aug 21, 2024

Uh oh!

hebasto commented Aug 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 22, 2024

Uh oh!

hebasto commented Aug 22, 2024

Uh oh!

hebasto commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 28, 2024

Uh oh!

fanquake commented Oct 1, 2024

Uh oh!

hebasto commented Oct 11, 2024

Uh oh!

fanquake commented Oct 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

build: Mark `x86_64-linux-gnu` release binaries as CET-enabled #30685

build: Mark `x86_64-linux-gnu` release binaries as CET-enabled #30685

hebasto commented Aug 20, 2024 •

edited

Loading

DrahtBot commented Aug 20, 2024 •

edited

Loading

hebasto Aug 22, 2024 •

edited

Loading

hebasto commented Aug 22, 2024 •

edited

Loading