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] Fix typo in relocation name #255

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

smithp35
Copy link
Contributor

@smithp35 smithp35 commented Apr 2, 2024

As pointed out in #253 the R_AARCH64_AUTH_GOT_LO12_NC is meant to be the AUTH variant of R_AARCH64_LD64_GOT_LO12_NC. As there is also a
R_AARCH64_LD32_GOT_LO12_NC relocation rename the relocation to R_AARCH64_LD64_AUTH_GOT_LO12_NC.

These relocations are in the appendix as we are currently expecting the GOT to be RELRO and unsigned in most signing schemas.

As pointed out in ARM-software#253
the R_AARCH64_AUTH_GOT_LO12_NC is meant to be the AUTH variant of
R_AARCH64_LD64_GOT_LO12_NC. As there is also a
R_AARCH64_LD32_GOT_LO12_NC relocation rename the relocation to
R_AARCH64_LD64_AUTH_GOT_LO12_NC.

These relocations are in the appendix as we are currently
expecting the GOT to be RELRO and unsigned in most signing schemas.
@MaskRay
Copy link
Contributor

MaskRay commented Apr 2, 2024

Shall we use GDAT(S) + A ? But that can be done after #217 has a concrete resolution.

@kovdan01
Copy link

kovdan01 commented Apr 2, 2024

I've just noticed that RAARCH64_AUTH_GOT_ADD_LO12_NC also contains a typo - _ is missed after R.
BTW, is there a corresponding non-auth reloc? I don't see R_AARCH64_GOT_ADD_LO12_NC in https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst

@smithp35
Copy link
Contributor Author

smithp35 commented Apr 3, 2024

I've just noticed that RAARCH64_AUTH_GOT_ADD_LO12_NC also contains a typo - _ is missed after R. BTW, is there a corresponding non-auth reloc? I don't see R_AARCH64_GOT_ADD_LO12_NC in https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst

I've updated to remove that relocation. I can't see anything it would be used for apart from calculating the address of a GOT slot, but user-code shouldn't be able to do that anyway and the compiler has other ways to do that if it needs to using the other relocations.

@smithp35
Copy link
Contributor Author

smithp35 commented Apr 3, 2024

Shall we use GDAT(S) + A ? But that can be done after #217 has a concrete resolution.

I've got an action to make an internal proposal for what to do about #217 this week.

@kovdan01
Copy link

@smithp35

I can't see anything it would be used for apart from calculating the address of a GOT slot, but user-code shouldn't be able to do that anyway

Is it actually true that we shouldn't be able to do that? According to the spec, address of the GOT entry is used as a modifier when signing the contents of the entry. The example code for accessing signed GOT entry includes this calculation (see https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst#default-signing-schema):

adrp x8, :got_auth: symbol
add x8, x8, :got_auth_lo12: symbol
ldr x0, [x8]
// Authenticate to get unsigned pointer
autia x0, x8

If R_AARCH64_AUTH_GOT_ADD_LO12_NC is removed, I suppose the code snippet should be changed correspondingly (not to include add with :got_auth_lo12: immediate). Personally, I feel that adrp+add is the most simple way to achieve the desired functionality, so the reloc should probably be left in the spec, just with the typo fixed. Please let me know if I miss smth and there is an easier way to re-implement the code block above.

@smithp35
Copy link
Contributor Author

@smithp35

I can't see anything it would be used for apart from calculating the address of a GOT slot, but user-code shouldn't be able to do that anyway

Is it actually true that we shouldn't be able to do that? According to the spec, address of the GOT entry is used as a modifier when signing the contents of the entry. The example code for accessing signed GOT entry includes this calculation (see https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst#default-signing-schema):

adrp x8, :got_auth: symbol
add x8, x8, :got_auth_lo12: symbol
ldr x0, [x8]
// Authenticate to get unsigned pointer
autia x0, x8

If R_AARCH64_AUTH_GOT_ADD_LO12_NC is removed, I suppose the code snippet should be changed correspondingly (not to include add with :got_auth_lo12: immediate). Personally, I feel that adrp+add is the most simple way to achieve the desired functionality, so the reloc should probably be left in the spec, just with the typo fixed. Please let me know if I miss smth and there is an easier way to re-implement the code block above.

Thanks for reminding me, a lot of the rationale has fallen out of my memory in the time that it was first wrote. I agree with you and this is how the AArch64 PAC PLTs are implemented in LLD, albeit with just the calculation and not the relocation.

In this particular case there is no need to rename the relocation as the ABI only uses the LD64 and LD32 when there is a material difference in the relocation. The R_AARCH64_AUTH_GOT_ADD_LO12_NC has no overflow check and would result
in bits [11:0] regardless of whether we were ILP32 and ILP64. The closest equivalent in the existing AAELF64 is R_<CLS>_ ADD_ABS_LO12_NC.

I'll update to put the relocation back in as was, and add a note below to match the relocation with the operator :got_auth_lo12:

There is no equivalent for this relocation in the standard ABI
it is used by runtime code to calculate the address of
a GOT slot so it can be used as one of the inputs to an
authenticate instruction. Add a note that this matches up
with the :got_auth_lo12: operator for future reference.

Part of ARM-software#253
@smithp35 smithp35 force-pushed the pauthgotgenerating branch from f58056e to c8a6c5d Compare April 15, 2024 12:29
kovdan01 added a commit to kovdan01/llvm-project that referenced this pull request Jul 1, 2024
@asl
Copy link

asl commented Jul 2, 2024

@smithp35 Thanks! Looks like there was some discrepancy in the particular implementation. Now everything fully matches.

@smithp35
Copy link
Contributor Author

smithp35 commented Jul 2, 2024

Thanks for the confirmation. Merging.

@smithp35 smithp35 merged commit 01f97f3 into ARM-software:main Jul 2, 2024
1 check passed
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