Issue #20154 has been updated by katei (Yuta Saito). Right, the `configure.ac` should respect user given CFLAGS as much as possible. I will make follow-up fixes to avoid overriding the option, but I don't think we can get it merged in 3.3.x branch since the overriding issue is not an obvious regression. (3.2.x also has the same problem, IIUC) Anyway, your patch in the downstream seems reasonable to me as a temporal fix. ---------------------------------------- Bug #20154: aarch64: configure overrides `-mbranch-protection` if it was set in CFLAGS via environment https://bugs.ruby-lang.org/issues/20154#change-106061 * Author: jprokop (Jarek Prokop) * Status: Open * Priority: Normal * ruby -v: ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [aarch64-linux] * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- Recently a GH PR was merged <https://github.com/ruby/ruby/pull/9306> For PAC/BTI support on ARM CPUs for Coroutine.S. Without proper compilation support in configure.ac it segfaults Ruby with fibers on CPUs where PAC is supported: https://bugs.ruby-lang.org/issues/20085 At the time of writing, configure.ac appends the first option from a list for flag `-mbranch-protection` that successfully compiles a program <https://github.com/ruby/ruby/blob/master/configure.ac#L829>, to XCFLAGS and now also ASFLAGS to fix issue 20085 for Ruby master. This is suboptimal for Fedora as we set -mbranch-protection=standard by default in C{,XX}FLAGS: ``` CFLAGS='-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Werror=implicit-function-declaration -Werror=implicit-int -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -mbranch-protection=standard -fasynchronous-unwind-tables -fstack-clash-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer ' export CFLAGS CXXFLAGS='-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -mbranch-protection=standard -fasynchronous-unwind-tables -fstack-clash-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer' export CXXFLAGS ``` And the appended flag overrides distribution's compilation configuration, which in this case ends up omitting BTI instructions and only using PAC. Would it make sense to check if such flags exist and not overwrite them if they do? Serious proposals: 1. Simplest fix that does not overwrite what is set in the distribution and results in higher security is simply prepending the list of options with `-mbranch-protection=standard`, it should cause no problems on ARMv8 CPUs and forward, BTI similarly to PAC instructions result into NOP, it is only extending the capability. See attached 0001-aarch64-Check-mbranch-protection-standard-first-to-u.patch 2. Other fix that sounds more sane IMO and dodges this kind of guessing where are all the correct places for the flag is what another Fedora contributor Florian Weimer suggested: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org… "The reliable way to do this would be to compile a C file and check whether that enables __ARM_FEATURE_PAC_DEFAULT, and if that's the case, define a *different* macro for use in the assembler implementation. This way, you don't need to care about the exact name of the option." IOW instead of using __ARM_FEATURE_* directly in that code, define a macro in the style of "USE_PAC" with value of the feature if it is defined, I think that way we shouldn't need to append ASFLAGS anymore. However it's also important to catch the value of those macros as their values have meaning, I have an idea how to do that but I'd get on that monday earliest. ---Files-------------------------------- 0001-aarch64-Check-mbranch-protection-standard-first-to-u.patch (1004 Bytes) -- https://bugs.ruby-lang.org/

Issue #20154 has been updated by kjtsanaktsidis (KJ Tsanaktsidis). I don't think I quite understand what exactly the right course of action here is. Why is this not a serious proposal? "We should respect user given CFLAGS as much as possible" sounds like a good philosophy, so this seems sensible? (I thought about just compiling a test program and seeing if it had __ARM_FEATURE_PAC_DEFAULT/__ARM_FEATURE_BTI_DEFAULT set, but that would not be able to tell if the user explicitly provided `-mbranch-protection=none`, and _that_ also should be respected). I'm not 100% sure, but is Florian suggesting here that: * passing `-mbranch-protector=` to the assembler doesn't actually _do_ anything other than defining the macros, * So our build system should just define its own macros by doing something like `AC_CHECK_DECLS(__ARM_FEATURE_PAC_DEFAULT)` etc and defining `RUBY_AARCH64_PAC` etc as a result * And the assembly files should check `RUBY_AARCH64_PAC` instead of `__ARM_FEATURE_PAC_DEFAULT` * And that this is better because distributions etc will put `-mbranch-protection=pac-ret+bti` in their CFLAGS, but not normally in their ASFLAGS (because this does nothing anyway) However doing some research it seems that other projects also apply their branch protection options to ASFLAGS e.g. https://source.chromium.org/chromium/chromium/src/+/main:build/config/linux…. So i'm not super sure about this. Should distros not also put `-mbranch-protection` in their ASFLAGS if they want them system-wide? (And also for e.g. rust https://doc.rust-lang.org/beta/unstable-book/compiler-flags/branch-protecti…) It seems that based on https://developer.arm.com/documentation/101028/0012/5--Feature-test-macros?… that * `__ARM_FEATURE_BTI_DEFAULT` is either undefined or 1 if BTI is enabled, so we don't need its meaning * `__ARM_FEATURE_PAC_DEFAULT` bits give a meaning though, and our current assembly in Coroutine.S totally ignores these meanings * Bits 0 and 1 determine which key to use. At the moment we sign with the A-key only. I don't know why PAC has two keys, but if a distro sets `-mbranch-protection=pac-ret+b-key`, we're going to be ignoring that. * Bit 2 determines if leaf functions that don't set up a stack frame should be protected. coroutine_transfer should always be protected so i don't think we need any information from this bit * Bit 3 determines if we should also add PC to the signature (I think? it means "Protection using PC as a diversifier"). This is for the FEAT_PAuth_LR extension which is new (https://github.com/ARM-software/acle/pull/292) and I don't know if any hardware actually has it yet? I don't know if this is something we would need to implement for coroutine_transfer or not. Maybe it wouldn't even make sense because the whole point of coroutine_transfer is to return to a different place than it was called from. Maybe we should just `#error` if the B-key is selected and leave it at that; if someone has a use-case then it can be implemented? ---- I've written a lot of text here but it boils down to "I'm not sure what we should do". Perhaps we should rip out _all_ branch-protection flags out of configure.ac and leave it to distributors to specify the CFLAGS/ASFLAGS that they want? ---------------------------------------- Bug #20154: aarch64: configure overrides `-mbranch-protection` if it was set in CFLAGS via environment https://bugs.ruby-lang.org/issues/20154#change-108333 * Author: jprokop (Jarek Prokop) * Status: Open * ruby -v: ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [aarch64-linux] * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- Recently a GH PR was merged <https://github.com/ruby/ruby/pull/9306> For PAC/BTI support on ARM CPUs for Coroutine.S. Without proper compilation support in configure.ac it segfaults Ruby with fibers on CPUs where PAC is supported: https://bugs.ruby-lang.org/issues/20085 At the time of writing, configure.ac appends the first option from a list for flag `-mbranch-protection` that successfully compiles a program <https://github.com/ruby/ruby/blob/master/configure.ac#L829>, to XCFLAGS and now also ASFLAGS to fix issue 20085 for Ruby master. This is suboptimal for Fedora as we set -mbranch-protection=standard by default in C{,XX}FLAGS: ``` CFLAGS='-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Werror=implicit-function-declaration -Werror=implicit-int -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -mbranch-protection=standard -fasynchronous-unwind-tables -fstack-clash-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer ' export CFLAGS CXXFLAGS='-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -mbranch-protection=standard -fasynchronous-unwind-tables -fstack-clash-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer' export CXXFLAGS ``` And the appended flag overrides distribution's compilation configuration, which in this case ends up omitting BTI instructions and only using PAC. Would it make sense to check if such flags exist and not overwrite them if they do? Serious proposals: 1. Simplest fix that does not overwrite what is set in the distribution and results in higher security is simply prepending the list of options with `-mbranch-protection=standard`, it should cause no problems on ARMv8 CPUs and forward, BTI similarly to PAC instructions result into NOP, it is only extending the capability. See attached 0001-aarch64-Check-mbranch-protection-standard-first-to-u.patch 2. Other fix that sounds more sane IMO and dodges this kind of guessing where are all the correct places for the flag is what another Fedora contributor Florian Weimer suggested: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org… "The reliable way to do this would be to compile a C file and check whether that enables __ARM_FEATURE_PAC_DEFAULT, and if that's the case, define a *different* macro for use in the assembler implementation. This way, you don't need to care about the exact name of the option." IOW instead of using __ARM_FEATURE_* directly in that code, define a macro in the style of "USE_PAC" with value of the feature if it is defined, I think that way we shouldn't need to append ASFLAGS anymore. However it's also important to catch the value of those macros as their values have meaning, I have an idea how to do that but I'd get on that monday earliest. ---Files-------------------------------- 0001-aarch64-Check-mbranch-protection-standard-first-to-u.patch (1004 Bytes) -- https://bugs.ruby-lang.org/