Skip to content

[PAC][lld] Fix reloc against adrp imm in PAC PLT header #113429

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

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

kovdan01
Copy link
Contributor

The PAC PLT header contains adrp instruction which immediate should be filled. In https://reviews.llvm.org/D62609, the adrp instruction address was calculated incorrectly. This patch resolves the issue.

The test is already present in test/ELF/aarch64-feature-pac.s

The PAC PLT header contains adrp instruction which immediate should be
filled. In https://reviews.llvm.org/D62609, the adrp instruction address
was calculated incorrectly. This patch resolves the issue.

The test is already present in test/ELF/aarch64-feature-pac.s
@kovdan01 kovdan01 requested review from asl, MaskRay and smithp35 October 23, 2024 08:25
@kovdan01 kovdan01 marked this pull request as ready for review October 23, 2024 08:25
@llvmbot
Copy link
Member

llvmbot commented Oct 23, 2024

@llvm/pr-subscribers-lld-elf

Author: Daniil Kovalev (kovdan01)

Changes

The PAC PLT header contains adrp instruction which immediate should be filled. In https://reviews.llvm.org/D62609, the adrp instruction address was calculated incorrectly. This patch resolves the issue.

The test is already present in test/ELF/aarch64-feature-pac.s


Full diff: https://github.com/llvm/llvm-project/pull/113429.diff

1 Files Affected:

  • (modified) lld/ELF/Arch/AArch64.cpp (+1-1)
diff --git a/lld/ELF/Arch/AArch64.cpp b/lld/ELF/Arch/AArch64.cpp
index f4f867d019136e..28e0fce6a6f499 100644
--- a/lld/ELF/Arch/AArch64.cpp
+++ b/lld/ELF/Arch/AArch64.cpp
@@ -1048,7 +1048,7 @@ void AArch64BtiPac::writePltHeader(uint8_t *buf) const {
   memcpy(buf, pltData, sizeof(pltData));
 
   relocateNoSym(buf + 4, R_AARCH64_ADR_PREL_PG_HI21,
-                getAArch64Page(got + 16) - getAArch64Page(plt + 8));
+                getAArch64Page(got + 16) - getAArch64Page(plt + 4));
   relocateNoSym(buf + 8, R_AARCH64_LDST64_ABS_LO12_NC, got + 16);
   relocateNoSym(buf + 12, R_AARCH64_ADD_ABS_LO12_NC, got + 16);
   if (!btiHeader)

@llvmbot
Copy link
Member

llvmbot commented Oct 23, 2024

@llvm/pr-subscribers-lld

Author: Daniil Kovalev (kovdan01)

Changes

The PAC PLT header contains adrp instruction which immediate should be filled. In https://reviews.llvm.org/D62609, the adrp instruction address was calculated incorrectly. This patch resolves the issue.

The test is already present in test/ELF/aarch64-feature-pac.s


Full diff: https://github.com/llvm/llvm-project/pull/113429.diff

1 Files Affected:

  • (modified) lld/ELF/Arch/AArch64.cpp (+1-1)
diff --git a/lld/ELF/Arch/AArch64.cpp b/lld/ELF/Arch/AArch64.cpp
index f4f867d019136e..28e0fce6a6f499 100644
--- a/lld/ELF/Arch/AArch64.cpp
+++ b/lld/ELF/Arch/AArch64.cpp
@@ -1048,7 +1048,7 @@ void AArch64BtiPac::writePltHeader(uint8_t *buf) const {
   memcpy(buf, pltData, sizeof(pltData));
 
   relocateNoSym(buf + 4, R_AARCH64_ADR_PREL_PG_HI21,
-                getAArch64Page(got + 16) - getAArch64Page(plt + 8));
+                getAArch64Page(got + 16) - getAArch64Page(plt + 4));
   relocateNoSym(buf + 8, R_AARCH64_LDST64_ABS_LO12_NC, got + 16);
   relocateNoSym(buf + 12, R_AARCH64_ADD_ABS_LO12_NC, got + 16);
   if (!btiHeader)

Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

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

LGTM.

This was discussed in Discord under https://discord.com/channels/636084430946959380/1133112394701348895/1298121731428585564

I think the existing incorrect value of 8 is always going to round down to the same page [1] as 4 by getAArch64Page, but it is worth getting this right in the source code.

[1] PLT is 16-byte aligned. I don't think there's a placement of the PLT such that PLT + 4 or PLT +8 can cross a 4KiB page boundary.

@kovdan01 kovdan01 merged commit a4ace3d into llvm:main Oct 23, 2024
13 checks passed
@frobtech frobtech mentioned this pull request Oct 25, 2024
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
The PAC PLT header contains adrp instruction which immediate should be
filled. In https://reviews.llvm.org/D62609, the adrp instruction address
was calculated incorrectly. This patch resolves the issue.

The test is already present in test/ELF/aarch64-feature-pac.s
@kovdan01 kovdan01 self-assigned this Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants