diff mbox series

aarch64: Fix PLT fixups when BTI is used [PR32572]

Message ID 20250123130714.3332660-1-rearnsha@arm.com
State New
Headers
Series aarch64: Fix PLT fixups when BTI is used [PR32572] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Richard Earnshaw Jan. 23, 2025, 1:07 p.m. UTC 
  PR ld/32572

There are two problems addressed in this PR.  Firstly, the choice of
whether or not a PLT stub needs a BTI on entry was too strict,
resulting in non-pie executables not having a BTI on their stub.  But
secondly, the logic to handle each stub types did not agree across the
various places where this information is used.

The first issue is fixed by using bfd_link_executable rather than
bfd_link_pde.  The second is addressed by recording a delta for PLT
stub alongside the stub itself.  This is then used without needing
additional logic later on since it has been pre-calculated.

A more comprehensive fix would involve creating a data structure to
describe each fixup, including a call-back function to apply any
relocations.  But that's a fairly large change and not appropriate for
backporting.
---

Yury has run this patch through the GLIBC testsuite and confirmed that
it fixes all the failures there.

I'll wait 24hrs before applying, though in case anybody else has some
comments.

R.

 bfd/elfnn-aarch64.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Aurelien Jarno Jan. 24, 2025, 8:31 a.m. UTC | #1 
Hi,

On 2025-01-23 13:07, Richard Earnshaw wrote:
> PR ld/32572
> 
> There are two problems addressed in this PR.  Firstly, the choice of
> whether or not a PLT stub needs a BTI on entry was too strict,
> resulting in non-pie executables not having a BTI on their stub.  But
> secondly, the logic to handle each stub types did not agree across the
> various places where this information is used.
> 
> The first issue is fixed by using bfd_link_executable rather than
> bfd_link_pde.  The second is addressed by recording a delta for PLT
> stub alongside the stub itself.  This is then used without needing
> additional logic later on since it has been pre-calculated.
> 
> A more comprehensive fix would involve creating a data structure to
> describe each fixup, including a call-back function to apply any
> relocations.  But that's a fairly large change and not appropriate for
> backporting.
> ---
> 
> Yury has run this patch through the GLIBC testsuite and confirmed that
> it fixes all the failures there.
> 
> I'll wait 24hrs before applying, though in case anybody else has some
> comments.
> 
> R.
> 
>  bfd/elfnn-aarch64.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)

Thanks a lot for this patch. I have tested it, I have found no
regression on the binutils testsuite, and I confirm it fixes the two
glibc tests when it is built with -mbranch-protection=standard.

Regards,
Aurelien
Richard Earnshaw (lists) Jan. 24, 2025, 3 p.m. UTC | #2 
On 24/01/2025 08:31, Aurelien Jarno wrote:
> Hi,
> 
> On 2025-01-23 13:07, Richard Earnshaw wrote:
>> PR ld/32572
>>
>> There are two problems addressed in this PR.  Firstly, the choice of
>> whether or not a PLT stub needs a BTI on entry was too strict,
>> resulting in non-pie executables not having a BTI on their stub.  But
>> secondly, the logic to handle each stub types did not agree across the
>> various places where this information is used.
>>
>> The first issue is fixed by using bfd_link_executable rather than
>> bfd_link_pde.  The second is addressed by recording a delta for PLT
>> stub alongside the stub itself.  This is then used without needing
>> additional logic later on since it has been pre-calculated.
>>
>> A more comprehensive fix would involve creating a data structure to
>> describe each fixup, including a call-back function to apply any
>> relocations.  But that's a fairly large change and not appropriate for
>> backporting.
>> ---
>>
>> Yury has run this patch through the GLIBC testsuite and confirmed that
>> it fixes all the failures there.
>>
>> I'll wait 24hrs before applying, though in case anybody else has some
>> comments.
>>
>> R.
>>
>>  bfd/elfnn-aarch64.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> Thanks a lot for this patch. I have tested it, I have found no
> regression on the binutils testsuite, and I confirm it fixes the two
> glibc tests when it is built with -mbranch-protection=standard.
> 
> Regards,
> Aurelien
> 

Thanks for checking.  I've now pushed this to master and the binutils-2.44 branch.

R.
diff mbox series

Patch

diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
index 1fe0b615800..9b8b86fd5d5 100644
--- a/bfd/elfnn-aarch64.c
+++ b/bfd/elfnn-aarch64.c
@@ -2631,6 +2631,11 @@  struct elf_aarch64_link_hash_table
   /* The bytes of the subsequent PLT entry.  */
   const bfd_byte *plt_entry;
 
+  /* PLT entries have a common shape, but may have some pre-amble
+     instructions (such as BTI).  This delta is used to factor this
+     out of the common code.  */
+  int plt_entry_delta;
+
   /* For convenience in allocate_dynrelocs.  */
   bfd *obfd;
 
@@ -2924,6 +2929,7 @@  elfNN_aarch64_link_hash_table_create (bfd *abfd)
   ret->plt_header_size = PLT_ENTRY_SIZE;
   ret->plt0_entry = elfNN_aarch64_small_plt0_entry;
   ret->plt_entry_size = PLT_SMALL_ENTRY_SIZE;
+  ret->plt_entry_delta = 0;
   ret->plt_entry = elfNN_aarch64_small_plt_entry;
   ret->tlsdesc_plt_entry_size = PLT_TLSDESC_ENTRY_SIZE;
   ret->obfd = abfd;
@@ -4959,15 +4965,17 @@  setup_plt_values (struct bfd_link_info *link_info,
       globals->plt0_entry = elfNN_aarch64_small_plt0_bti_entry;
 
       /* Only in ET_EXEC we need PLTn with BTI.  */
-      if (bfd_link_pde (link_info))
+      if (bfd_link_executable (link_info))
 	{
 	  globals->plt_entry_size = PLT_BTI_PAC_SMALL_ENTRY_SIZE;
 	  globals->plt_entry = elfNN_aarch64_small_plt_bti_pac_entry;
+	  globals->plt_entry_delta = 4;
 	}
       else
 	{
 	  globals->plt_entry_size = PLT_PAC_SMALL_ENTRY_SIZE;
 	  globals->plt_entry = elfNN_aarch64_small_plt_pac_entry;
+	  globals->plt_entry_delta = 0;
 	}
     }
   else if (plt_type == PLT_BTI)
@@ -4975,10 +4983,11 @@  setup_plt_values (struct bfd_link_info *link_info,
       globals->plt0_entry = elfNN_aarch64_small_plt0_bti_entry;
 
       /* Only in ET_EXEC we need PLTn with BTI.  */
-      if (bfd_link_pde (link_info))
+      if (bfd_link_executable (link_info))
 	{
 	  globals->plt_entry_size = PLT_BTI_SMALL_ENTRY_SIZE;
 	  globals->plt_entry = elfNN_aarch64_small_plt_bti_entry;
+	  globals->plt_entry_delta = 4;
 	}
     }
   else if (plt_type == PLT_PAC)
@@ -9841,11 +9850,9 @@  elfNN_aarch64_create_small_pltn_entry (struct elf_link_hash_entry *h,
   /* Copy in the boiler-plate for the PLTn entry.  */
   memcpy (plt_entry, htab->plt_entry, htab->plt_entry_size);
 
-  /* First instruction in BTI enabled PLT stub is a BTI
-     instruction so skip it.  */
-  if (elf_aarch64_tdata (output_bfd)->sw_protections.plt_type & PLT_BTI
-      && elf_elfheader (output_bfd)->e_type == ET_EXEC)
-    plt_entry = plt_entry + 4;
+  /* Allow for any delta (such as a BTI instruction) before the common
+     sequence.  */
+  plt_entry += htab->plt_entry_delta;
 
   /* Fill in the top 21 bits for this: ADRP x16, PLT_GOT + n * 8.
      ADRP:   ((PG(S+A)-PG(P)) >> 12) & 0x1fffff */