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

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:240820-control-flow changing 3 files +15 −1
  1. hebasto commented at 5:39 pm on August 20, 2024: member

    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 @ d79ea809d28197b1b4e3748aa1715272b53601d0, the release binaries are not marked as CET-enabled:

    0$ env HOSTS=x86_64-linux-gnu ./contrib/guix/guix-build
1$ tar -xf guix-build-d79ea809d281/output/x86_64-linux-gnu/bitcoin-d79ea809d281-x86_64-linux-gnu.tar.gz
2$ readelf -n bitcoin-d79ea809d281/bin/bitcoind | grep -A 5 "\.note\.gnu\.property"
3Displaying notes found in: .note.gnu.property
4  Owner                Data size 	Description
5  GNU                  0x00000020	NT_GNU_PROPERTY_TYPE_0
6      Properties: x86 feature used: x86, x87, XMM, YMM, XSAVE
7	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:

    0$ env HOSTS=x86_64-linux-gnu ./contrib/guix/guix-build
1$ tar -xf guix-build-c5bed747e6e9/output/x86_64-linux-gnu/bitcoin-c5bed747e6e9-x86_64-linux-gnu.tar.gz
2$ readelf -n bitcoin-c5bed747e6e9/bin/bitcoind | grep -A 6 "\.note\.gnu\.property"
3Displaying notes found in: .note.gnu.property
4  Owner                Data size 	Description
5  GNU                  0x00000030	NT_GNU_PROPERTY_TYPE_0
6      Properties: x86 feature: IBT, SHSTK
7	x86 feature used: x86, x87, XMM, YMM, XSAVE
8	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):

    0$ export GLIBC_TUNABLES=glibc.cpu.hwcaps=SHSTK
1$ bitcoin-c5bed747e6e9/bin/bitcoind -printtoconsole=0 &
2$ cat /proc/$(cat .bitcoin/bitcoind.pid)/status | grep x86
3x86_Thread_features:	shstk 
4x86_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.

  2. hebasto added the label Build system on Aug 20, 2024
  3. DrahtBot commented at 5:39 pm on August 20, 2024: contributor

    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.

  4. fanquake commented at 5:52 pm on August 20, 2024: member

    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.

  5. DrahtBot added the label CI failed on Aug 20, 2024
  6. DrahtBot commented at 6:37 pm on August 20, 2024: contributor

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

    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.

  7. hebasto commented at 9:48 am on August 21, 2024: member

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

    Is that branch publicly available?

  8. hebasto marked this as a draft on Aug 21, 2024
  9. hebasto force-pushed on Aug 21, 2024
  10. hebasto marked this as ready for review on Aug 21, 2024
  11. DrahtBot removed the label CI failed on Aug 21, 2024
  12. hebasto commented at 7:49 pm on August 21, 2024: member

    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).

    1. CI is green.
    2. This approach works and fixes #30677.
    3. #30438 does not indicate that it is capable of resolving #30677.
  13. in depends/hosts/linux.mk:37 in 89f8e8e7f5 outdated 
    32@@ -33,6 +33,11 @@ x86_64_linux_AR=ar
33 x86_64_linux_RANLIB=ranlib
34 x86_64_linux_NM=nm
35 x86_64_linux_STRIP=strip
36+
37+ifeq ($(NO_HARDEN),)
    fanquake commented at 7:58 pm on August 21, 2024:
    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.
    sipa commented at 8:15 pm on August 21, 2024:

    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).

    fanquake commented at 8:25 pm on August 21, 2024:

    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.

    sipa commented at 8:29 pm on August 21, 2024:

    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?

    fanquake commented at 10:08 am on August 22, 2024:

    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).

  14. in contrib/guix/libexec/build.sh:234 in 89f8e8e7f5 outdated 
    229@@ -230,6 +230,10 @@ case "$HOST" in
230     *mingw*)  HOST_LDFLAGS="-Wl,--no-insert-timestamp" ;;
231 esac
232 
233+case "$HOST" in
234+    x86_64-linux-gnu)  HARDENED_LDFLAGS="-Wl,-z,cet-report=error" ;;
    fanquake commented at 10:10 am on August 22, 2024:
    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.
    hebasto commented at 11:20 am on August 22, 2024:
    This is done to prevent disruption of ~internal flag checks~ test-security-check.py.
    fanquake commented at 11:21 am on August 22, 2024:
    What disruption?
    hebasto commented at 12:38 pm on August 22, 2024: 
     0x86_64-linux-gnu-ld: /tmp/ccwqGS4u.o: error: missing IBT and SHSTK properties
 1collect2: error: ld returned 1 exit status
 2E
 3======================================================================
 4ERROR: test_ELF (__main__.TestSecurityChecks)
 5----------------------------------------------------------------------
 6Traceback (most recent call last):
 7  File "/distsrc-base/distsrc-5fda052191b0-x86_64-linux-gnu/./contrib/devtools/test-security-check.py", line 67, in test_ELF
 8    self.assertEqual(call_security_check(cxx, source, executable, pass_flags + ['-fcf-protection=none']), (1, executable + ': failed CONTROL_FLOW'))
 9  File "/distsrc-base/distsrc-5fda052191b0-x86_64-linux-gnu/./contrib/devtools/test-security-check.py", line 42, in call_security_check
10    subprocess.run([*cxx,source,'-o',executable] + env_flags() + options, check=True)
11  File "/gnu/store/fn4rrnx7vx1bl6mccrhm95y6br8yg9s6-python-minimal-3.10.7/lib/python3.10/subprocess.py", line 524, in run
12    raise CalledProcessError(retcode, process.args,
    fanquake commented at 12:43 pm on August 22, 2024:
    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.
    hebasto commented at 12:51 pm on August 22, 2024:
    OK. What can you suggest?
    fanquake commented at 9:28 am on September 23, 2024:

    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.

  15. fanquake commented at 11:10 am on August 22, 2024: member

    Guix building 89f8e8e7f5828b1d06925e036c16eda050b05c81 on aarch64 doesn’t work:

    0time HOSTS="x86_64-linux-gnu" ./contrib/guix/guix-build
1<snip>
2  CXXLD    test/test_bitcoin
3x86_64-linux-gnu-ld: /bitcoin/depends/x86_64-linux-gnu/lib/libevent.a(buffer.c.o): error: missing IBT and SHSTK properties
4x86_64-linux-gnu-ld: /bitcoin/depends/x86_64-linux-gnu/lib/libevent.a(bufferevent.c.o): error: missing IBT and SHSTK properties
5<snip>
6x86_64-linux-gnu-ld: /bitcoin/depends/x86_64-linux-gnu/lib/libzmq.a(mechanism_base.cpp.o): error: missing IBT and SHSTK properties
7collect2: error: ld returned 1 exit status
  16. depends: Enable control-flow protection x86 Linux targets a8c08ef32e
  17. guix: Enable CET for `glibc` package 636fba1067
  18. guix: Check for IBT and SHSTK properties in .note.gnu.property section c5bed747e6
  19. hebasto force-pushed on Aug 22, 2024
  20. hebasto commented at 1:58 pm on August 22, 2024: member
    The PR description has been updated with a runtime check example.
  21. hebasto commented at 3:36 pm on August 22, 2024: member

    Guix building 89f8e8e on aarch64 doesn’t work:

    The issue should be fixed now.

  22. DrahtBot added the label Needs rebase on Aug 28, 2024
  23. DrahtBot commented at 9:56 am on August 28, 2024: contributor

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

  24. fanquake commented at 12:58 pm on October 1, 2024: member
    What is the status of this?
  25. hebasto commented at 7:03 pm on October 11, 2024: member
    Apparently, I haven’t had time to address feedback and rebase this PR recently. Perhaps it could be labelled “Up for grabs”?
  26. hebasto marked this as a draft on Oct 11, 2024
  27. fanquake commented at 2:05 pm on October 16, 2024: member

    Perhaps it could be labelled “Up for grabs”?

    I’ll cherry-pick / followup with this.

  28. fanquake closed this on Oct 16, 2024


hebasto DrahtBot fanquake sipa

Labels
Build system Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-18 07:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me