ARM-software / abi-aa

Application Binary Interface for the Arm® Architecture
Other
938 stars 188 forks source link

[pauthabielf64] Fix typo in relocation name #255

Closed smithp35 closed 3 months ago

smithp35 commented 6 months ago

As pointed out in https://github.com/ARM-software/abi-aa/issues/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 commented 6 months ago

Shall we use GDAT(S) + A ? But that can be done after https://github.com/ARM-software/abi-aa/issues/217 has a concrete resolution.

kovdan01 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

@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 commented 5 months ago

@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:

asl commented 3 months ago

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

smithp35 commented 3 months ago

Thanks for the confirmation. Merging.