Flags of the module derived exclusively from the compiler flag -mbranch-protection.
The note is generated based on the module flags accordingly.
After this change in case of compile unit without function won't have
the .note.gnu.property if the compiler flag is not present [1].
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Should the per-function analysis warning actually be removed? That seems like a helpful check to catch a different form of bad behavior.
Comment Actions
Specifically, this appears to be a legitimate bug, found by the warnings: https://bugs.llvm.org/show_bug.cgi?id=46258
Comment Actions
Function level attributes could set different attributes for functions. If function attribute is used then I assume the user know what he/she is doing so no need to emit a warning.
Maybe some would ensure the function is only directly called and never called indirectly by enforcing the branch-proctection=None or Pac-ret.
By default this warning shall not present anyway, especially for LLVM internals.
Specifically, this appears to be a legitimate bug, found by the warnings: https://bugs.llvm.org/show_bug.cgi?id=46258
Thanks for pointing to this bug, I pick it up, since my two other patches address it:
https://reviews.llvm.org/D75181
and for CFI https://reviews.llvm.org/D81251
after those two patches, if we would emit a warning for the different flags we could do it in clang due to no reason to postpone the diagnostic to the backend. also would be easier suppress the warning if needed.
Comment Actions
Might someone wish to disable PAC/BTI on an individual function, while having it on for the rest? I guess that would mean you can't call that function indirectly?
Comment Actions
It would mean you can't call it _at all_, not just indirectly. :) Which is why I still think the warning is useful. Perhaps don't warn for the functions with the attribute?
Comment Actions
BTI landing pad is needed only when the previous instructions was an indirect branch/jump bl x16.
With a direct branch bl foo no landing pad is needed at all. Rational is that direct branches can't be hijacked, they always land at the same location.
Landing pads emitted only when indirect branch is possible to a function.[1]
Comment Actions
I don't think this behaviour is correct with regard to the specification (AAELF64 2020Q2):
Static linkers processing ELF relocatable objects must set the feature bit in the output object or image
only if all the input objects have the corresponding feature bit set.
GNU_PROPERTY_AARCH64_FEATURE_1_BTI This indicates that all executable sections are compatible with
Branch Target Identification mechanism. An executable or shared object with this bit set is required to
generate Custom PLTs (page 35) with BTI instruction.GNU_PROPERTY_AARCH64_FEATURE_1_PAC This indicates that all executable sections have Return Address
Signing enabled. An executable or shared object with this bit set can generate Custom PLTs (page 35)
with a PAC instruction.
Compatibility of executable sections ultimately depends on each individual function, therefore
it cannot be inferred from command-line options alone (transitively from module flags), which
merely set a default that can be overridden by function attributes.
If any function has the attribute "sign-return-address", then the output note
section should have PAC bit set. The return address signing is completely local
to the function, and functions with or without return address signing can be
freely mixed with each other.
Likewise, if any function has the attribute "branch-target-enforcement", then
the output note section should have the BTI flag set. Even though code compiled
with BTI is not necessarily compatible with non-BTI code:
- the only way to get BTI code is by explicit use of -mbranch-protection=... command-line option, or the corresponding attribute, which we should consider a clear indication about the user's intent to use BTI.
- the only way to get a mix of present/non-present "branch-target-enforcement" attributes is by the explicit use of the __attribute__((target("branch-protection=...")), in which case we should assume the user knows what they are doing.
What do to if there are no functions in the compile unit?
Technically, objects produced from such a unit are fully compatible with both PAC and BTI, which
means both flags should be set. But looking at the (non-existent) function attributes alone does
not allow us to unambiguously derive a user's intent to use PAC/BTI. In this case, I would suggest
setting the ELF note flags, according to the LLVM IR module flags.
Comment Actions
If any function has the attribute "sign-return-address", then the output note
section should have PAC bit set. The return address signing is completely local
to the function, and functions with or without return address signing can be
freely mixed with each other.
That is true PAC and non-PAC functions can be mixed.
Does one function makes the "all executable sections" pac-ret enabled?
BTW GNU_PROPERTY_AARCH64_FEATURE_1_PAC is not really used for anything.
Likewise, if any function has the attribute "branch-target-enforcement", then
the output note section should have the BTI flag set. Even though code compiled
with BTI is not necessarily compatible with non-BTI code:
- the only way to get BTI code is by explicit use of -mbranch-protection=... command-line option, or the corresponding attribute, which we should consider a clear indication about the user's intent to use BTI.
- the only way to get a mix of present/non-present "branch-target-enforcement" attributes is by the explicit use of the __attribute__((target("branch-protection=...")), in which case we should assume the user knows what they are doing.
__ARM_FEATURE_PAC_DEFAULT and __ARM_FEATURE_BTI_DEFAULT controlled by the -mbranch-protection=...
https://developer.arm.com/documentation/101028/0011/Feature-test-macros?lang=en
One of the reasons of the introduction of these macros is the management of the function attributes.
For example:
#ifdef __ARM_FEATURE_PAC_DEFAULT
#ifdef __ARM_FEATURE_BTI_DEFAULT
#define NO_PAC_FUNC __attribute__((target("branch-protection=bti")))
#else
#define NO_PAC_FUNC __attribute__((target("branch-protection=none")))
#endif /* __ARM_FEATURE_BTI_DEFAULT */
...In my humble opinion the function attribute is there to alter global setting.
I considered to propagate the function attribute to the module flags but
that would lead to inconsistent compilation with the macros that I'd avoid.
What do to if there are no functions in the compile unit?
Technically, objects produced from such a unit are fully compatible with both PAC and BTI, which
means both flags should be set. But looking at the (non-existent) function attributes alone does
not allow us to unambiguously derive a user's intent to use PAC/BTI. In this case, I would suggest
setting the ELF note flags, according to the LLVM IR module flags.
I think the only clear indication from the user to use PAC/BTI is the explicit use of -mbranch-protection=... command-line option.
A few function attributes that would turn PAC/BTI on just on those few functions makes no sense for me in any real world application.
Valid to turn off PAC/BTI on selected functions while the whole application compiled with them.
We need to turn PAC off on the code path where we change\manage the keys for example.
Exaggerated example for BTI: https://godbolt.org/z/Y9bhe9 Current version of llvm issues a warning and won't emit the note while I think it should.
Comment Actions
Yes, the presence of even a single function is a clear indication of what the user whats - enable PAC/BTI.
The default is not having PAC/BTI code gen, therefore its presence is a result of a deliberate action by the user,
therefore unambiguously conveys the user's intent.
BTW GNU_PROPERTY_AARCH64_FEATURE_1_PAC is not really used for anything.
I may not be used today in GNU/Linux, but still, it has to have sensible semantics.
One of the reasons of the introduction of these macros is the management of the function attributes.
For example:#ifdef __ARM_FEATURE_PAC_DEFAULT #ifdef __ARM_FEATURE_BTI_DEFAULT #define NO_PAC_FUNC __attribute__((target("branch-protection=bti"))) #else #define NO_PAC_FUNC __attribute__((target("branch-protection=none"))) #endif /* __ARM_FEATURE_BTI_DEFAULT */ ...
I don't see how this example is relevant to the discussion of what notes to emit.
Our starting point is we have some default state (in module flags or whatever), some
individual function state and we have to decide what notes to emit, regardless of the specific way
we came up with those function attributes.
In my humble opinion the function attribute is there to alter global setting.
I considered to propagate the function attribute to the module flags but
that would lead to inconsistent compilation with the macros that I'd avoid.
The compilation of a single given function does not necessarily need to be
consistent with the value of these macros. Quite the opposite really, the macros themselves are
suffixed by _DEFAULT in order to explicitly acknowledge that possibility.
What do to if there are no functions in the compile unit?
Technically, objects produced from such a unit are fully compatible with both PAC and BTI, which
means both flags should be set. But looking at the (non-existent) function attributes alone does
not allow us to unambiguously derive a user's intent to use PAC/BTI. In this case, I would suggest
setting the ELF note flags, according to the LLVM IR module flags.I think the only clear indication from the user to use PAC/BTI is the explicit use of -mbranch-protection=... command-line option.
Using the attribute is no less clear and even carries more weight, as it overrides the command line option.
A few function attributes that would turn PAC/BTI on just on those few functions makes no sense for me in any real world application.
Turning on/off PAC/BTI is completely symmetrical - one can achieve exactly the same effect with:
- command-line options enabling PAC/BTI and individual attributes disabling BTI
- command-line options disabling PAC/BIT (e.g. not having a command-line option at all) and individual attributes enabling it
We shouldn't be guessing and prescribing how applications should use the mechanisms we make available and certainly
shouldn't be judging what is a real-world application and what is not.
Valid to turn off PAC/BTI on selected functions while the whole application compiled with them.
We need to turn PAC off on the code path where we change\manage the keys for example.
Exaggerated example for BTI: https://godbolt.org/z/Y9bhe9 Current version of llvm issues a warning and won't emit the note while I think it should.
Just as valid is to turn on PAC/BTI on selected functions, while the while compilation unit (*not* application) is compiled without them.
Comment Actions
This version of the patch behaves as gcc for case when no function present and when function has -mbranch-protection attribute without compiler flag.
The logic should be in clang because in llvm we won't have enough information to handle these things. (see D75181)
Comment Actions
the gcc behaviour is not exactly ideal, but it's better if llvm is compatible with it or fix gcc if something is broken there.
the assumption is that the intended branch protection is implied via cmdline flags for the tu and function attributes are only used in source code for some hack. a common reason for such hack is to disable bti somewhere but still keep the bti elf marking. (if the intention was to mark the code non-bti compatible then just dont compile it with bti, using a non-portable bti specfic function attribute would not work well for such use anyway)
now this may not work well with lto when functions can come from different places and the function attributes encode how those were compiled. so hopefully there is something that preserves the flags of the translation units and explicitly specified function attributes can be treated separately (such that they dont affect elf markings).
Comment Actions
I don't share this assumption. I find it just as valid to control the PAC/BTI with things like:
#ifdef ENABLE_BTI
#define BTI_FUNC __attribute__((target("branch-protection=bti")))
#else
#define BTI_FUNC
BTI_FUNC void foo() { ...
BTI_FUNC int bar() { ...without using any command-line option other than -DENABLE_BTI=1.
Comment Actions
The 08/10/2020 10:03, Momchil Velikov via Phabricator wrote:
chill added a comment.
In D80791#2196598 https://reviews.llvm.org/D80791#2196598, @nsz wrote:
the assumption is that the intended branch protection is implied via cmdline flags for the tu and function attributes are only used in source code for some hack.
I don't share this assumption. I find it just as valid to control the PAC/BTI with things like:
#ifdef ENABLE_BTI #define BTI_FUNC __attribute__((target("branch-protection=bti"))) #else #define BTI_FUNC BTI_FUNC void foo() { ... BTI_FUNC int bar() { ...without using any command-line option other than -DENABLE_BTI=1.
i think that cannot work.
the implementation is free to inject arbitrary code into
user code so if the user does not tell the implementation
that it wants the entire tu to be bti safe then non-bti
code can end up in there. (e.g. ctor of an instrumentation
that is not realated to any particular function with the
bti marking)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80791/new/
--
Comment Actions
Certainly, there are cases it won't work, but there are definitely
cases where it *can* work. Whatever the implementation does
should be a deterministic consequence of implementing the relevant
language standards together with implementation-defined behaviour,
command-line options and language extensions (e..g attributes).
Certainly I don't expect C++ ctorts/dtors in C code and gcov or
sanitiser calls if I haven't given relevant -fprofile-whatever/-fsanitize=whatever
options. In that sense, the implementation cannot do whatever
it pleases, it is constrained to a range of behaviours one can reason about.
Comment Actions
i think it's a bad idea to use function level
attributes to control what markings we attach to
translation units and i would prefer to only add
markings to object files when the compiler is
asked to do so per tu.
i dont want to see source code changes
to enable bti, that should be only needed
for some special case that's way out of
standard conform code.
Comment Actions
I would prefer to avoid the situation where the markings of two otherwise identical files were different,
depending on how the files were produced, no matter if it was a common or a special case.
Comment Actions
i don't see why it is desirable to silently get marking on an object file if function definitions happen to be bti compatible in it:
- compiler cannot reliably do this (e.g. bti incompatible inline asm).
- some users don't want the marking: not all linkers support it so it can cause unexpected breakage.
- most users (all?) want the marking reliably (not opportunistically), but function annotations are fragile (can depend on optimizations and code outside of user control).
- it is not useful to have a bti annotated function unless everything else is bti compatible too: it is all or nothing per elf module.
- but a compiler cannot diagnose if only some functions have the annotation (we don't have a cflag for it) so even if the compiler tried to add the marking silently users cannot rely on it: it's too easy to drop the marking and no way to debug such failure.
Comment Actions
It is desirable to get the marking because BTI-compatible functions don't appear by accident - they are a result of deliberate user actions, which clearly
express intent to use BTI.
- compiler cannot reliably do this (e.g. bti incompatible inline asm).
Like for any other case, that's entirely the responsibility of the user if they use inline asm; Command-line options are not
special with regard to inline asm, so everything that can break attribute-derived marking, breaks command-line derived marking.
- some users don't want the marking: not all linkers support it so it can cause unexpected breakage.
Those linkers would need to be upgraded if the compiler imposes extra requirements on them. One can't hold the compiler hostage to obsolete linkers. If users insist, they can just remove the .note section.
- most users (all?) want the marking reliably (not opportunistically), but function annotations are fragile (can depend on optimizations and code outside of user control).
The user explicitly marking an object is the least reliable option, because it's done without regard what the object in actually contains.
- it is not useful to have a bti annotated function unless everything else is bti compatible too: it is all or nothing per elf module.
This is false. Some functions in an elf module could be in a guarded region, some in a non-guarded region. Some function may always
be called in a "BTI-safe" way, which may be unknown to the compiler.
- but a compiler cannot diagnose if only some functions have the annotation (we don't have a cflag for it) so even if the compiler tried to add the marking silently users cannot rely on it: it's too easy to drop the marking and no way to debug such failure.
At the time a compiler decides to or decides not to emit instructions which implement PAC-RET or BTI is perfectly clear what;s the effective annotation for each individual function.
I don't really understand the point of all these objections.
With my proposal to derive marking from function attributes, as well as from command-line
everything above will still work in the (arguably) most common case that we expect - users just using
command line.
I'm proposing to be strict and cover a few corner case where the command-line only approach produces bogus results.
Comment Actions
the current design is per elf module, so the non-guarded things have to go into a different elf module (and thus different tu).
i think the only way the current abi supports mixing bti and non-bti code is if all the linker inputs to the elf module are marked as bti compatible and then the user explicitly unprotects some region at runtime, i.e. bti is still all or nothing per elf module, but a user might want to do some hack and turn bti off in some places.
With my proposal to derive marking from function attributes, as well as from command-line
everything above will still work in the (arguably) most common case that we expect - users just using
command line.I'm proposing to be strict and cover a few corner case where the command-line only approach produces bogus results.
ok i think deriving the marking in the absence of command-line option works, but it's not something users can rely on and not what gcc does.
Comment Actions
it is not useful to have a bti annotated function unless everything else is bti compatible too: it is all or nothing per elf module.
This is false. Some functions in an elf module could be in a guarded region, some in a non-guarded region. Some function may always
be called in a "BTI-safe" way, which may be unknown to the compiler.
Right now the elf and all of the text sections considered BTI enabled or not. The dynamic linkers/loaders can't support this use case without additional information to be encoded somewhere (and specified). To support such we need to consider grouping/align to page boundaries these functions in the linker because BTI could be controlled by flags in PTE.
With the current spec this usecase is not supported in this way. The user have to link the BTI protected code into another elf.
Side note: The force-bti linker option can't work with half BTI enabled objects.
Comment Actions
I suppose this is valid for typical Linux-based systems today.
Is it valid in general, across the whole spectre of operating systems or for bare-metal targets?
Guess not.
Comment Actions
the linker may or may not need to generate code (PLT is just one example) and the current abi is designed such that it is an elf module level decision if that code is bti compatible or not.
this is why i said that the current abi does not support mixed bti and non-bti code, however the user can do this if lies to the linker that everything is bti compatible so the linker generates stub code accordingly.
i don't see how baremetal can get away here: this is elf abi.
Comment Actions
In D85649 I suggested a different version of module flags, which is a bit nicer to use, e.g. one can say just
getModuleFlag("sign-return-address-with-bkey") != nullptrinstead of a) checking for the flag presence, b) getting its value and c) comparing it to a set of strings, which is
way too verbose.
Thus, the set of module flags are essentially booleans:
- "sign-return-address" when PAC-RET is enabled; it establishes the defaults of signing non-leaf functions with the A key
- "sign-return-address-all", modifies the default, established by "sign-return-address" to signing all functions, including ones that do not spill LR
- "sign-return-address-with-bkey", modifies the default, established by "sign-return-address" to signing with the B key.
These are not ABI, so if, in the future, if we do need a set of values, we can easily change it.
Comment Actions
In D85649 I changed the module flags to be always present and have a zero/non-zero value. That's needed during LTO, if a flag is present in one module and absent in another,
no error is reported and the existing flags is used in the merged module, affecting the codegen for the module that did not initially have the flag.
tl;dr we need to check the value of the flags, not just their existence.