Skip to content

[Bug #20154] Extract hardening CFLAGS to a special $hardenflags variable #10944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 11, 2024
Merged

[Bug #20154] Extract hardening CFLAGS to a special $hardenflags variable #10944

merged 1 commit into from
Jun 11, 2024

Conversation

KJTsanaktsidis
Copy link
Contributor

This changes the automatic detection of -fstack-protector, -D_FORTIFY_SOURCE, and -mbranch-protection to write to $hardenflags instead of $XCFLAGS. The definition of $cflags is changed to "$hardenflags $orig_cflags $optflags $debugflags $warnflags" to match.

Furthermore, these flags are prepended to $hardenflags, rather than appended.

The implications of doing this are as follows:

  • If a CRuby builder specifies cflags="-mbranch-protection=foobar" at the ./configure script, and the configure script detects that -mbranch-protection=pac-ret is accepted, then GCC will be invoked as "gcc -mbranch-protection=pac-ret -mbranch-protection=foobar". Since the last flags take precedence, that means that user-supplied values of these flags in $cflags will take priority.
  • Likewise, if a CRuby builder explicitly specifies "hardenflags=-mbranch-protection=foobar", because we prepend to $hardenflags in our autoconf script, we will still invoke GCC as "gcc -mbranch-protection=pac-ret -mbranch-protection=foobar".
  • If a CRuby builder specifies CFLAGS="..." at the configure line, automatic detection of hardening flags is ignored as before.
  • C extensions will also be built with hardening flags now as well (this was not the case by default before because the detected flags went into $XCFLAGS).

Additionally, as part of this work, I changed how the detection of PAC/BTI in Context.S works. Rather than appending the autodetected option to ASFLAGS, we simply compile a set of test programs with the actual CFLAGS in use to determine what PAC/BTI settings were actually chosen by the builder. Context.S is made aware of these choices through some custom macros.

The result of this work is that:

  • Ruby will continue to choose some sensible defaults for hardening options for the C compiler
  • Distributors are able to specify CFLAGS that are consistent with their distribution and override these defaults
  • Context.S will react to whatever -mbranch-protection is actually in use, not what was autodetected
  • Extensions get built with hardening flags too.

[Bug #20154]
[Bug #20520]

@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/autoconf_fix_branch_protection branch from 7db9c7a to 094384b Compare June 9, 2024 11:33
@KJTsanaktsidis KJTsanaktsidis marked this pull request as ready for review June 9, 2024 23:47
@KJTsanaktsidis
Copy link
Contributor Author

@nobu - You're the expert on Autoconf - does this look good to you? 🙏
@ioquatix - the Context.S changes - are you OK with this too?

Thanks!

@KJTsanaktsidis KJTsanaktsidis requested review from nobu and ioquatix June 9, 2024 23:48
ioquatix
ioquatix approved these changes Jun 10, 2024
View reviewed changes
Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

LGTM!

nobu
nobu reviewed Jun 10, 2024
View reviewed changes
configure.ac Outdated
# That means this is the right time to check what branch protection flags are going to be in use
# and define appropriate macros for use in Context.S based on this
AS_CASE(["$target_cpu"], [aarch64], [
AC_MSG_CHECKING(whether __ARM_FEATURE_BTI_DEFAULT is defined)
Copy link
Member

Choose a reason for hiding this comment

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

Check every time, not using AC_CACHE_CHECK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually didn't think about caching. but, thinking about it now, it should be uncached - these defines will change depending on the cflags used. (Or does the autoconf cache invalidate if cflags are changed? I’ll try it tomorrow)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good news, looks like autoconf is smart enough to bust the cache here.

ktsanaktsidis@lima-fedora build % ../configure  --prefix=$(realpath ~/rubies/3.4.0-master) --disable-install-doc --disable-yjit cflags="-mbranch-protection=standard" --config-cache
configure: loading cache config.cache
configure: error: `cflags' has changed since the previous run:
configure:   former value:  `-mbranch-protection=none'
configure:   current value: `-mbranch-protection=standard'
configure: error: in `/home/ktsanaktsidis.linux/ruby/build':
configure: error: changes in the environment can compromise the build
configure: error: run `make distclean' and/or `rm config.cache'
	    and start over

So i'll wrap it in AC_CACHE_CHECK.

@KJTsanaktsidis
Extract hardening CFLAGS to a special $hardenflags variable
3d40563 
This changes the automatic detection of -fstack-protector,
-D_FORTIFY_SOURCE, and -mbranch-protection to write to $hardenflags
instead of $XCFLAGS. The definition of $cflags is changed to
"$hardenflags $orig_cflags $optflags $debugflags $warnflags" to match.

Furthermore, these flags are _prepended_ to $hardenflags, rather than
appended.

The implications of doing this are as follows:

* If a CRuby builder specifies cflags="-mbranch-protection=foobar" at
  the ./configure script, and the configure script detects that
  -mbranch-protection=pac-ret is accepted, then GCC will be invoked as
  "gcc -mbranch-protection=pac-ret -mbranch-protection=foobar". Since
  the last flags take precedence, that means that user-supplied values
  of these flags in $cflags will take priority.
* Likewise, if a CRuby builder explicitly specifies
  "hardenflags=-mbranch-protection=foobar", because we _prepend_ to
  $hardenflags in our autoconf script, we will still invoke GCC as
  "gcc -mbranch-protection=pac-ret -mbranch-protection=foobar".
* If a CRuby builder specifies CFLAGS="..." at the configure line,
  automatic detection of hardening flags is ignored as before.
* C extensions will _also_ be built with hardening flags now as well
  (this was not the case by default before because the detected flags
  went into $XCFLAGS).

Additionally, as part of this work, I changed how the detection of
PAC/BTI in Context.S works. Rather than appending the autodetected
option to ASFLAGS, we simply compile a set of test programs with the
actual CFLAGS in use to determine what PAC/BTI settings were actually
chosen by the builder. Context.S is made aware of these choices through
some custom macros.

The result of this work is that:

* Ruby will continue to choose some sensible defaults for hardening
  options for the C compiler
* Distributors are able to specify CFLAGS that are consistent with their
  distribution and override these defaults
* Context.S will react to whatever -mbranch-protection is actually in
  use, not what was autodetected
* Extensions get built with hardening flags too.

[Bug #20154]
[Bug #20520]
@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/autoconf_fix_branch_protection branch from 094384b to 3d40563 Compare June 11, 2024 08:59
@KJTsanaktsidis KJTsanaktsidis merged commit 0ccb80d into ruby:master Jun 11, 2024
99 of 100 checks passed
@KJTsanaktsidis KJTsanaktsidis deleted the ktsanaktsidis/autoconf_fix_branch_protection branch June 11, 2024 10:49
@ioquatix
Copy link
Member

Nice work, the simplifications to Context.S are meaningful.

KJTsanaktsidis added a commit that referenced this pull request Jul 6, 2024
@KJTsanaktsidis
Revert autoconf macros defining RUBY_AARCH64_{BTI|PAC}_ENABLED
b13a097 
This partially reverts #10944; now that
we decided to pass CFLAGS to $(CC) when assembling .S files, we don't
need these autoconf macros that capture the state of
__ARM_FEATURE{PAC|BTI}_DEFAULT.

[Bug #20601]
KJTsanaktsidis added a commit that referenced this pull request Jul 6, 2024
@KJTsanaktsidis
Revert autoconf macros defining RUBY_AARCH64_{BTI|PAC}_ENABLED
975c53d 
This partially reverts #10944; now that
we decided to pass CFLAGS to $(CC) when assembling .S files, we don't
need these autoconf macros that capture the state of
__ARM_FEATURE{PAC|BTI}_DEFAULT.

[Bug #20601]
KJTsanaktsidis added a commit that referenced this pull request Jul 7, 2024
@KJTsanaktsidis
Revert autoconf macros defining RUBY_AARCH64_{BTI|PAC}_ENABLED
468c022 
This partially reverts #10944; now that
we decided to pass CFLAGS to $(CC) when assembling .S files, we don't
need these autoconf macros that capture the state of
__ARM_FEATURE{PAC|BTI}_DEFAULT.

[Bug #20601]
KJTsanaktsidis added a commit that referenced this pull request Jul 7, 2024
@KJTsanaktsidis
Revert autoconf macros defining RUBY_AARCH64_{BTI|PAC}_ENABLED
b940de8 
This partially reverts #10944; now that
we decided to pass CFLAGS to $(CC) when assembling .S files, we don't
need these autoconf macros that capture the state of
__ARM_FEATURE{PAC|BTI}_DEFAULT.

[Bug #20601]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nobu nobu nobu approved these changes

@ioquatix ioquatix ioquatix approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KJTsanaktsidis @ioquatix @nobu