Skip to content
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.

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

[PAuthABIELF64] Use .note.gnu.property section as ELF marking scheme. #240

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

smithp35
Copy link
Contributor

Make the alternative .note.gnu.property section the default ELF marking scheme for ELF executables and shared libraries. Preserve the original SHT_NOTE section as an alternative for legacy compatibility.

Tighten up the wording to restrict the property to just a pair of 64-bit words. This permits the program property to be described by a pair of build attributes which Arm plans to introduce for relocatable ELF object marking.

@asl
Copy link

asl commented Jan 29, 2024

Tagging @kovdan01

the first 64-bit word being a platform identifier, and the second
64-bit word being a version number for the ABI for the platform
identified for the first word. When ``descsz`` is larger than 16 the
remainder of the contents of desc are defined by the (platform id,

Choose a reason for hiding this comment

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

For new default marking schema (via GNU property section), you've changed the format so the property only contains a <platform, version> tuple (total 16 bytes). Have you left an option for additional data (more than 16 bytes) in alternative marking schema intentionally or is this just a copy-paste mistake and this should also be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that was deliberate as I wanted to try and keep the relocatable object part simple, i.e. just (platform id, version). That makes it simple for an upstream linker, to know what to do. If there is arbitrary platform specific information then it is difficult for upstream to know what to do about it. It also makes it easier to use the two build attributes I've defined in #230 Tag_PAuth_Platform and Tag_PAuth_Schema .

If you're using that field for something or plan to, please let me know and I'll work something out.

I think what I'd most likely do is say something like:

  • Any platform specific information is determined by the (platform id, version), the format and interpretation of this data is outside the scope of this specification. This information may be ignored by a tool if the platform id and version are unknown.

Choose a reason for hiding this comment

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

Yes, thanks, the rationale for only keeping platform and version without extra data is clear.

What I'm trying to ask is why the alternative ELF marking schema still allows extra data? I mean the following (see phrases in bold):

The descsz field must be at least 16. See desc below.

And the following:

When descsz is larger than 16 the remainder of the contents of desc are defined by the (platform id, version number).

ABI.

.note.gnu.properties are defined for relocatable objects. Arm intends
to use build-attributes for relocatable-object ELF marking. This will

Choose a reason for hiding this comment

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

Have I understood this correct: this document is to be extended in future after #230 gets merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have I understood this correct: this document is to be extended in future after #230 gets merged?

Yes. #230 contains a document that reserves the Tag_PAuth_Platform and Tag_PAuth_Schema which map to the platform id and version number for platform id.

Once a build attributes implementation has been merged, Arm plans to do this as part of the Guarded Control Stack work, I will update the document to link to the build attributes specification.

For the existing .note.gnu.properties defined in PAuthABI and in Sysvabi the intention is to define the build attributes in a compatible way such that a linker can accept build attributes or a .note.gnu.property section from relocatable objects.

.note.gnu.properties are defined for relocatable objects. Arm intends
to use build-attributes for relocatable-object ELF marking. This will
be defined so that there is a 1:1 mapping between the program
property and the build-attributes for PAuthABI.

Base Compatibility Model

Choose a reason for hiding this comment

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

Don't we need to change this part as well? It now describes the compatibility model for ELF marking based on .note.AARCH64-PAUTH-ABI-tag section.

BTW, it would also be nice to explicitly say if files using different ELF marking ways (but with the same <platform, version> tuple, meaning the same signing schema) should be considered compatible or not. It was somewhere discussed previously and thinking of such files as compatible ones was considered as unneeded complexity - I'm just saying that explicit mention of this in this document would be nice to have IMHO.

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'll need to take a look. I wanted to get a draft out before the Monday so it is possible I've missed something.

I'll add a note to state that the key information is <platform, version> no matter how they are encoded. Will do that before next Monday.

@smithp35
Copy link
Contributor Author

I've made an update that tries to extract the core parts of any ELF marking into a separate section. This permits multiple encodings of that same information.

I've written of the compatibility model as possible using the core information.

I expect I'll need several rounds of review to get this update right. I'm also aware I'll need to proofread all that I've written again as I wanted to get this update out before the end of the day.

@asl
Copy link

asl commented Mar 4, 2024

Just to note: the current implementation in LLVM is done according to this spec.

@smithp35
Copy link
Contributor Author

smithp35 commented Mar 5, 2024

I'll set a review deadline for 12/05/2024, I'll merge unless there are any objections.

file contains pointers, they were signed in a compatible way with
the default signing rules for tuple (platform id, version number).
* The absence of any ELF marking means no information on how pointers
are signed is available for this ELF file. When used in combination

Choose a reason for hiding this comment

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

When used in combination
with ELF files that contain ELF marking, then the file is assigned
the (platform identifer, version number) of (0,0).

I think we should allow different behavior here. A user might want static linker to "ignore" such files with no compatibility info and treat them as compatible with any others. See similar option -z bti-report in lld https://github.com/llvm/llvm-project/blob/546f32df26f58fdfe02d99e6d91d681dd9ed6839/lld/docs/ld.lld.1#L749. It might be helpful if we already have some pre-built pauth-disabled binaries and if we are sure that using them won't break pauth-enabled code from other binaries. For example, in a C/C++ project, we might have an assembly file manually defining a section or smth, and it would be nice not to include pauth compatibility info inside it manually as well.

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've added a clarifying sentence that states that implementations can be provided with supplemental information that can change this default.

Make the alternative .note.gnu.property section the default ELF
marking scheme for ELF executables and shared libraries. Preserve
the original SHT_NOTE section as an alternative for legacy
compatibility.

Extract the common parts of all of the ELF marking schemes
into "Core Information". The ELF marking schemes now describe
how they encode the "Core Information". The compatibility model
is now written in terms of the "Core Information".

The default PAuth vendor and version for ELF files with no ELF
marking is (0,0) which is incompatible with everything. This is
the safest default as there is no information about what, if any,
signing schema is used by that ELF file.

Implementations can change this default in the presence of
supplemental information, such as a linker command line option.
For example there could be the equivalent of the -z force-bti
which declares that all ELF objects without the BTI property are
considered to be compatible.
@smithp35
Copy link
Contributor Author

Squashing prior to rebase and merge.

@smithp35 smithp35 merged commit 8d5e422 into ARM-software:main Mar 15, 2024
1 check passed
@kovdan01
Copy link

It looks like that after merging this into main, the rst preview on GitHub stopped working. I tried to find the root cause manually and by utils checkrst and rst2html - they found no issues. This script from GitHub repo also converts the file to html w/o issues https://github.com/github/markup/blob/master/lib/github/commands/rest2html. So, unfortunately I can't say what is the particular source of the issue, but the issue itself exists.

Having rst preview working is needed to provide links to particular parts of the document, for example, in code comments (via #part-name ending in url).

@smithp35
Copy link
Contributor Author

We are aware of the general issue (#236). Several other ABI documents are affected.

From our investigation on bisecting changes there is a maximum character limit for the RST preview that was introduced by Github some Months ago. We've reported to Github (mentioned in
https://github.com/orgs/community/discussions/86715#discussioncomment-8155221). Github support have hinted that they have a solution coming soon, but we've no precise ETA of when that will be.

kovdan01 added a commit to llvm/llvm-project that referenced this pull request Apr 2, 2024
…RE_PAUTH` (#85231)

This adds support for `GNU_PROPERTY_AARCH64_FEATURE_PAUTH` feature (as
defined in ARM-software/abi-aa#240) handling in
llvm-readobj and llvm-readelf. The following constants for supported
platforms are also introduced:

- `AARCH64_PAUTH_PLATFORM_INVALID = 0x0`
- `AARCH64_PAUTH_PLATFORM_BAREMETAL = 0x1`
- `AARCH64_PAUTH_PLATFORM_LLVM_LINUX = 0x10000002`

For the llvm_linux platform, output of the tools contains descriptions
of PAuth features which are enabled/disabled depending on the version
value. Version value bits correspond to the following `LangOptions`
defined in #85232:

- bit 0: `PointerAuthIntrinsics`;
- bit 1: `PointerAuthCalls`;
- bit 2: `PointerAuthReturns`;
- bit 3: `PointerAuthAuthTraps`;
- bit 4: `PointerAuthVTPtrAddressDiscrimination`;
- bit 5: `PointerAuthVTPtrTypeDiscrimination`;
- bit 6: `PointerAuthInitFini`.

Support for `.note.AARCH64-PAUTH-ABI-tag` is dropped since it's deleted
from the spec in ARM-software/abi-aa#250.
kovdan01 added a commit to llvm/llvm-project that referenced this pull request Apr 4, 2024
…RE_PAUTH` (#87545)

Reland #85231 after fixing build failure
https://lab.llvm.org/buildbot/#/builders/186/builds/15631.
Use `PRIx64` for format output of `uint64_t` as hex.
Original PR description below.

This adds support for `GNU_PROPERTY_AARCH64_FEATURE_PAUTH` feature (as
defined in ARM-software/abi-aa#240) handling in
llvm-readobj and llvm-readelf. The following constants for supported
platforms are also introduced:

- `AARCH64_PAUTH_PLATFORM_INVALID = 0x0`
- `AARCH64_PAUTH_PLATFORM_BAREMETAL = 0x1`
- `AARCH64_PAUTH_PLATFORM_LLVM_LINUX = 0x10000002`

For the llvm_linux platform, output of the tools contains descriptions
of PAuth features which are enabled/disabled depending on the version
value. Version value bits correspond to the following `LangOptions`
defined in #85232:

- bit 0: `PointerAuthIntrinsics`;
- bit 1: `PointerAuthCalls`;
- bit 2: `PointerAuthReturns`;
- bit 3: `PointerAuthAuthTraps`;
- bit 4: `PointerAuthVTPtrAddressDiscrimination`;
- bit 5: `PointerAuthVTPtrTypeDiscrimination`;
- bit 6: `PointerAuthInitFini`.

Support for `.note.AARCH64-PAUTH-ABI-tag` is dropped since it's deleted
from the spec in ARM-software/abi-aa#250.
Copy link

@sallyarmneale sallyarmneale left a comment

Choose a reason for hiding this comment

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

No changes required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants