Skip to content

cc-wrapper: add zerocallusedregs hardening flag, add pkgsExtraHardening package set#274089

Merged
risicle merged 3 commits intoNixOS:stagingfrom
risicle:ris-zerocallusedregs
Jan 21, 2024
Merged

cc-wrapper: add zerocallusedregs hardening flag, add pkgsExtraHardening package set#274089
risicle merged 3 commits intoNixOS:stagingfrom
risicle:ris-zerocallusedregs

Conversation

@risicle
Copy link
Contributor

@risicle risicle commented Dec 13, 2023

Description of changes

More information on -fzero-call-used-regs: https://www.jerkeby.se/newsletter/posts/rop-reduction-zero-call-user-regs/

Short version: this hardening flag (available in gcc >= 11) reduces the number of ROP gadgets present in code by inserting instructions zeroing registers on function returns. The choice of used-gpr as a setting for regular use comes from the conclusions of the above article noting other projects making this choice.

This flag is also available in clang >= 15 but only for x86_64 & aarch64, and our hardeningUnsupportedFlags mechanism isn't sophisticated enough to manage variation by target platform (we could trivially vary hardeningUnsupportedFlags based on stdenv.targetPlatform, but that might produce the wrong result when called as a cross-compiler). So for now I've had to disable it for clang. (Perhaps hardeningUnsupportedFlags could accept a function taking a platform? let's not get too diverted..) Not such a problem after all, implemented, see comment below 👇

The more visible change in this PR is adding a new package set, pkgsExtraHardening, which I'd like to use as a testing ground for new hardening flags or flags which are still known to cause some problems. Users of other "hardened" features may be interested in using this package set. The name is a little clunky - I was also considering pkgsHarder (pkgsMeHarder? maybe not..)

Have tested bootstrapping pkgsExtraHardening on macos 12 x86_64, nixos x86_64 (+pkgsi686Linux, pkgsLLVM, pkgsCross.aarch64-multiplatform...). A couple of months ago I built quite a lot of nixos x86_64 with this flag set and struggled to find anything that had a problem with it.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@risicle risicle requested a review from a user December 13, 2023 22:59
@risicle risicle requested a review from Ericson2314 as a code owner December 13, 2023 22:59
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Dec 13, 2023
@risicle risicle added the 0.kind: enhancement Add something new or improve an existing system. label Dec 13, 2023
@risicle risicle force-pushed the ris-zerocallusedregs branch from ea132b5 to ac79e6b Compare December 13, 2023 23:08
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Dec 14, 2023
@RaitoBezarius
Copy link
Member

Two questions:

  • Does the new package scope gets built by Hydra?
  • What is the code size consequence on enabling those mitigations on a NixOS system? (Can we compare it?)

Other than that, I love that feature!

@risicle
Copy link
Contributor Author

risicle commented Dec 14, 2023

Does the new package scope gets built by Hydra?

Nope.

What is the code size consequence on enabling those mitigations on a NixOS system? (Can we compare it?)

Good question. Find out. (Would be good for more people than just me to have tried this...)

But seeing as this looks destined for being enabled by default in the kernel (at some point), I would have expected to hear more noise about it if there were a significant impact.

This is just the first of several new hardening flags I want to introduce in this release cycle, but am going to try and concentrate on one at a time.

@risicle risicle force-pushed the ris-zerocallusedregs branch from ac79e6b to f8340db Compare December 17, 2023 20:06
@risicle
Copy link
Contributor Author

risicle commented Dec 18, 2023

Ok, I've nerd-sniped myself into implementing the per-platform hardeningUnsupportedFlags. I've been reliably informed that a compiler's target is effectively fixed when it is cc-wrapper'ed, so we don't have to worry about e.g. runtime target detection, even for compilers like clang that are multi-target by nature.

I've explained more in the commit message, but this adds a new passthru attr, hardeningUnsupportedFlagsByTargetPlatform, to the affected clang derivations. Using a new attribute for this allows more straightforward overriding patterns, and continues to support hardeningUnsupportedFlags as long as the "innermost" hardeningUnsupportedFlagsByTargetPlatform implementation falls back to the package's hardeningUnsupportedFlags however it sees fit.

Tested successful bootstrap of pkgsExtraHardening on macos 12 x86_64 & nixos x86_64 pkgsLLVM.pkgsExtraHardening (and pkgsLLVM.pkgsStatic.pkgsExtraHardening)

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internals look fairly reasonable, but I'm not expert on those parts (but I went through those rabbit holes quite often).

I would like the stdenv (IIRC) documentation to be updated though with the new per-platform flag.

@risicle
Copy link
Contributor Author

risicle commented Dec 18, 2023

hardeningUnsupportedFlagsByTargetPlatform isn't an attr on stdenv, it's an attr on the compiler derivation - I don't think we've documented the interface between the compiler and cc-wrapper (e.g. hardeningUnsupportedFlags) before.

pkgsExtraHardening should probably get some documentation, and these changes should get added to the release notes - but I'd like to know if people are going to throw these ideas out (or ask for significant changes) before I go and write these.

Speaking of which #273840 could still do with a review.

@risicle risicle force-pushed the ris-zerocallusedregs branch from ee5e212 to 588a77a Compare December 19, 2023 23:50
@risicle
Copy link
Contributor Author

risicle commented Dec 26, 2023

@ofborg eval

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3151

@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label Dec 31, 2023
@risicle
Copy link
Contributor Author

risicle commented Jan 4, 2024

@ofborg eval

@risicle risicle mentioned this pull request Jan 4, 2024
12 tasks
@cole-h cole-h removed the ofborg-internal-error Ofborg encountered an error label Jan 4, 2024
@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label Jan 4, 2024
@cole-h cole-h removed the ofborg-internal-error Ofborg encountered an error label Jan 5, 2024
@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label Jan 5, 2024
@cole-h
Copy link
Member

cole-h commented Jan 5, 2024

As mentioned on matrix: ofborg is failing because:

This SHA and context has reached the maximum number of statuses.

If you force-push a new git sha, it'll start working again. Unfortunately, this is a GitHub limitation (maybe something happened during an outage that caused this), so that's the only workaround I can think of (aside from submitting a brand new PR altogether, which seems less ideal if a force-push would work).

It seems like the only way to get around this is to open a new PR that isn't burdened by the hundreds of commit statuses that GitHub's API takes issue with :/

@risicle risicle force-pushed the ris-zerocallusedregs branch from 588a77a to 30d5312 Compare January 5, 2024 22:18
@risicle
Copy link
Contributor Author

risicle commented Jan 6, 2024

@cole-h oh, could this be ofborg trying to extend its evaluation into pkgsExtraHardening? I think I just assumed that ofborg and hydra would ignore other package sets...

@cole-h cole-h removed the ofborg-internal-error Ofborg encountered an error label Jan 9, 2024
@risicle risicle force-pushed the ris-zerocallusedregs branch from 30d5312 to 6eb44d4 Compare January 12, 2024 21:10
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 13, 2024
@risicle risicle force-pushed the ris-zerocallusedregs branch from 6eb44d4 to e79388d Compare January 13, 2024 01:54
@ofborg ofborg bot added ofborg-internal-error Ofborg encountered an error and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jan 13, 2024
this uses the value `used-gpr` which seems to be a commonly
chosen value for general use
@risicle risicle force-pushed the ris-zerocallusedregs branch from e79388d to 0c3f41a Compare January 20, 2024 13:49
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes labels Jan 20, 2024
@risicle
Copy link
Contributor Author

risicle commented Jan 20, 2024

Added some release notes (and rebased again in the vain hope of pleasing ofborg)

@cole-h
Copy link
Member

cole-h commented Jan 21, 2024

I think you'll need to add pkgsExtraHardening to this list:

EDIT: Yeah, that let ofborg continue to do its thing on my test PR: #282387

this package set can be used to trial new hardening flags or
enable those which are still known to cause some problems
…lusedregs

this allows a compiler derivation to provide a
hardeningUnsupportedFlagsByTargetPlatform passthru attr
that will be called with the targetPlatform to determine
the unsupported hardening flags for that platform.

we can do this because even though a clang compiler is
multi-target by nature, cc-wrapper effectively fixes the
target platform at wrapping time. otherwise we'd have to
sniff the intended target at runtime, which wouldn't
be fun at all.

the advantage of using a new attribute instead of
allowing hardeningUnsupportedFlags to optionally be a
function is that hardeningUnsupportedFlags retains its
simple overriding pattern for simple cases (i.e.
  `(prev.hardeningUnsupportedFlags or []) ++ [ "foo" ]`
) which will continue to work as long as the bottom-most
function of hardeningUnsupportedFlagsByTargetPlatform
falls back to hardeningUnsupportedFlags.
@risicle risicle force-pushed the ris-zerocallusedregs branch from 0c3f41a to 506ec38 Compare January 21, 2024 11:16
@risicle risicle merged commit be19273 into NixOS:staging Jan 21, 2024
@cole-h cole-h removed the ofborg-internal-error Ofborg encountered an error label Jan 21, 2024
@cole-h cole-h mentioned this pull request Jan 21, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.kind: enhancement Add something new or improve an existing system. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: stdenv Standard environment 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants