mbox series

[0/5] aarch64 BTI stub fixes

Message ID cover.1699016830.git.szabolcs.nagy@arm.com
Headers
Series aarch64 BTI stub fixes |

Message

Szabolcs Nagy Nov. 3, 2023, 1:15 p.m. UTC 
  Large binaries with BTI can be linked incorrectly with
binutils 2.41 because PR30930.

there are other less critical related issues fixed in
this patchset as well.

Szabolcs Nagy (5):
  bfd: aarch64: Fix BTI stub optimization PR30957
  bfd: aarch64: Fix broken BTI stub PR30930
  bfd: aarch64: Fix leaks in case of BTI stub reuse
  bfd: aarch64: Avoid BTI stub for a PLT that has BTI
  ld: aarch64: Add BTI stub insertion test PR30930

 bfd/elfnn-aarch64.c                     | 89 +++++++++++++++++--------
 ld/testsuite/ld-aarch64/aarch64-elf.exp |  1 +
 ld/testsuite/ld-aarch64/bti-far-3.d     | 87 ++++++++++++++++++++++++
 ld/testsuite/ld-aarch64/bti-far-3.ld    | 12 ++++
 ld/testsuite/ld-aarch64/bti-far-3a.s    | 23 +++++++
 ld/testsuite/ld-aarch64/bti-far-3b.s    | 25 +++++++
 ld/testsuite/ld-aarch64/bti-far-3c.s    | 24 +++++++
 7 files changed, 232 insertions(+), 29 deletions(-)
 create mode 100644 ld/testsuite/ld-aarch64/bti-far-3.d
 create mode 100644 ld/testsuite/ld-aarch64/bti-far-3.ld
 create mode 100644 ld/testsuite/ld-aarch64/bti-far-3a.s
 create mode 100644 ld/testsuite/ld-aarch64/bti-far-3b.s
 create mode 100644 ld/testsuite/ld-aarch64/bti-far-3c.s

Comments

Nick Clifton Nov. 7, 2023, 11:38 a.m. UTC | #1 
Hi Szabolcs,

> Large binaries with BTI can be linked incorrectly with
> binutils 2.41 because PR30930.
> 
> there are other less critical related issues fixed in
> this patchset as well.
> 
> Szabolcs Nagy (5):
>    bfd: aarch64: Fix BTI stub optimization PR30957
>    bfd: aarch64: Fix broken BTI stub PR30930
>    bfd: aarch64: Fix leaks in case of BTI stub reuse
>    bfd: aarch64: Avoid BTI stub for a PLT that has BTI
>    ld: aarch64: Add BTI stub insertion test PR30930

This patch series looks good to me, so please apply if you have not already done so.


FYI - I did find one regression in the linker testsuite with the patch applied:

   FAIL: Check linker stubs with indirect calls handle BTI (exe).

However there are mitigating circumstances:

   * The failure was for an unusual toolchain configuration:

       --target=aarch64_be-linux-gnu_ilp32

   * The three other tests in this part of the testsuite also fail, both
     with and without your patch applied:

       FAIL: Check linker stubs with indirect calls handle BTI (shared lib).
       FAIL: Check linker stubs with indirect calls handle BTI (exe).
       FAIL: Check linker stubs with indirect calls handle BTI when target has BTI.

    Note: there appear to be *two* tests called "check linker stubs with indirect calls handle BTI (exe)."

I leave it up to you to decide if it is worth fixing these tests.

Cheers
   Nick

PS.  My gut feeling at the moment is that we do not need a 2.41.1 release
for this patch, since AArch64 systems with BTI enabled are not yet the norm,
and those that do exist are being used to test for problems like this rather
than general use.  Since your patch will be in the 2.42 release, due January
next year, I think that that will be a reasonable timescale for getting the
fix out there.  Do you agree ?
Szabolcs Nagy Nov. 7, 2023, 1:08 p.m. UTC | #2 
The 11/07/2023 11:38, Nick Clifton wrote:
> Hi Szabolcs,
> 
> > Large binaries with BTI can be linked incorrectly with
> > binutils 2.41 because PR30930.
> > 
> > there are other less critical related issues fixed in
> > this patchset as well.
> > 
> > Szabolcs Nagy (5):
> >    bfd: aarch64: Fix BTI stub optimization PR30957
> >    bfd: aarch64: Fix broken BTI stub PR30930
> >    bfd: aarch64: Fix leaks in case of BTI stub reuse
> >    bfd: aarch64: Avoid BTI stub for a PLT that has BTI
> >    ld: aarch64: Add BTI stub insertion test PR30930
> 
> This patch series looks good to me, so please apply if you have not already done so.
> 
> 
> FYI - I did find one regression in the linker testsuite with the patch applied:
> 
>   FAIL: Check linker stubs with indirect calls handle BTI (exe).
> 
> However there are mitigating circumstances:
> 
>   * The failure was for an unusual toolchain configuration:
> 
>       --target=aarch64_be-linux-gnu_ilp32
> 
>   * The three other tests in this part of the testsuite also fail, both
>     with and without your patch applied:
> 
>       FAIL: Check linker stubs with indirect calls handle BTI (shared lib).
>       FAIL: Check linker stubs with indirect calls handle BTI (exe).
>       FAIL: Check linker stubs with indirect calls handle BTI when target has BTI.
> 
>    Note: there appear to be *two* tests called "check linker stubs with indirect calls handle BTI (exe)."
> 
> I leave it up to you to decide if it is worth fixing these tests.

thanks for looking at this.
i will have to check what's going on.

> PS.  My gut feeling at the moment is that we do not need a 2.41.1 release
> for this patch, since AArch64 systems with BTI enabled are not yet the norm,
> and those that do exist are being used to test for problems like this rather
> than general use.  Since your patch will be in the 2.42 release, due January
> next year, I think that that will be a reasonable timescale for getting the
> fix out there.  Do you agree ?

makes sense,
unless it affects hardened kernel builds.
i will have to check if that's an issue.

thanks.
Szabolcs Nagy Nov. 9, 2023, 2:58 p.m. UTC | #3 
The 11/07/2023 13:08, Szabolcs Nagy wrote:
> The 11/07/2023 11:38, Nick Clifton wrote:
> > FYI - I did find one regression in the linker testsuite with the patch applied:
> > 
> >   FAIL: Check linker stubs with indirect calls handle BTI (exe).
> > 
> > However there are mitigating circumstances:
> > 
> >   * The failure was for an unusual toolchain configuration:
> > 
> >       --target=aarch64_be-linux-gnu_ilp32
> > 
> >   * The three other tests in this part of the testsuite also fail, both
> >     with and without your patch applied:
> > 
> >       FAIL: Check linker stubs with indirect calls handle BTI (shared lib).
> >       FAIL: Check linker stubs with indirect calls handle BTI (exe).
> >       FAIL: Check linker stubs with indirect calls handle BTI when target has BTI.
> > 
> >    Note: there appear to be *two* tests called "check linker stubs with indirect calls handle BTI (exe)."
> > 
> > I leave it up to you to decide if it is worth fixing these tests.
> 
> thanks for looking at this.
> i will have to check what's going on.

i will change the name of the new test to be unique.

cleaning up ilp32 test issues will be a fair bit of work.

i can commit a followup patch to force the tests i added to use
lp64 abi, like we do on other tests, but this is not ideal.

> > PS.  My gut feeling at the moment is that we do not need a 2.41.1 release
> > for this patch, since AArch64 systems with BTI enabled are not yet the norm,
> > and those that do exist are being used to test for problems like this rather
> > than general use.  Since your patch will be in the 2.42 release, due January
> > next year, I think that that will be a reasonable timescale for getting the
> > fix out there.  Do you agree ?
> 
> makes sense,
> unless it affects hardened kernel builds.
> i will have to check if that's an issue.

in practice i think the kernel should be fine.
(it's hard to say conclusively)

but i plan to backport the patches even if we don't arrange
a new release.
Fangrui Song Nov. 17, 2023, 8:42 a.m. UTC | #4 
On Thu, Nov 9, 2023 at 6:58 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 11/07/2023 13:08, Szabolcs Nagy wrote:
> > The 11/07/2023 11:38, Nick Clifton wrote:
> > > FYI - I did find one regression in the linker testsuite with the patch applied:
> > >
> > >   FAIL: Check linker stubs with indirect calls handle BTI (exe).
> > >
> > > However there are mitigating circumstances:
> > >
> > >   * The failure was for an unusual toolchain configuration:
> > >
> > >       --target=aarch64_be-linux-gnu_ilp32
> > >
> > >   * The three other tests in this part of the testsuite also fail, both
> > >     with and without your patch applied:
> > >
> > >       FAIL: Check linker stubs with indirect calls handle BTI (shared lib).
> > >       FAIL: Check linker stubs with indirect calls handle BTI (exe).
> > >       FAIL: Check linker stubs with indirect calls handle BTI when target has BTI.
> > >
> > >    Note: there appear to be *two* tests called "check linker stubs with indirect calls handle BTI (exe)."
> > >
> > > I leave it up to you to decide if it is worth fixing these tests.
> >
> > thanks for looking at this.
> > i will have to check what's going on.
>
> i will change the name of the new test to be unique.
>
> cleaning up ilp32 test issues will be a fair bit of work.
>
> i can commit a followup patch to force the tests i added to use
> lp64 abi, like we do on other tests, but this is not ideal.
>
> > > PS.  My gut feeling at the moment is that we do not need a 2.41.1 release
> > > for this patch, since AArch64 systems with BTI enabled are not yet the norm,
> > > and those that do exist are being used to test for problems like this rather
> > > than general use.  Since your patch will be in the 2.42 release, due January
> > > next year, I think that that will be a reasonable timescale for getting the
> > > fix out there.  Do you agree ?
> >
> > makes sense,
> > unless it affects hardened kernel builds.
> > i will have to check if that's an issue.
>
> in practice i think the kernel should be fine.
> (it's hard to say conclusively)
>
> but i plan to backport the patches even if we don't arrange
> a new release.

% grep '\.zero' ld/testsuite/ld-aarch64/bti-far*
ld/testsuite/ld-aarch64/bti-far-3a.s:.zero      0x07000000
ld/testsuite/ld-aarch64/bti-far-3b.s:.zero      0x01000000
ld/testsuite/ld-aarch64/bti-far-3b.s:.zero      0x07000000
ld/testsuite/ld-aarch64/bti-far-3c.s:.zero      0x01000000
% size ld/tmpdir/bti-far-*
   text    data     bss     dec     hex filename
117440552             0       0 117440552       7000028 ld/tmpdir/bti-far-3a.o
134217768             0       0 134217768       8000028 ld/tmpdir/bti-far-3b.o
16777260              0       0 16777260        100002c ld/tmpdir/bti-far-3c.o
    132       0       0     132      84 ld/tmpdir/bti-far-opt.o


`.zero      0x07000000` is 112MiB.  Perhaps use output section
addresses (e.g. [1]) to make the files smaller?

[1]: https://github.com/llvm/llvm-project/blob/de176d8c5496d6cf20e82aface98e102c593dbe2/lld/test/ELF/aarch64-thunk-pi.s#L112
Szabolcs Nagy Nov. 20, 2023, 2:56 p.m. UTC | #5 
The 11/17/2023 00:42, Fangrui Song wrote:
> On Thu, Nov 9, 2023 at 6:58 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> `.zero      0x07000000` is 112MiB.  Perhaps use output section
> addresses (e.g. [1]) to make the files smaller?

i cannot reproduce the original bug with different
output sections.

the bug requires the next input section after the
inserted stubs to be grouped with those stubs.
which i think does not happen in bfd ld if the next
input section goes to a different output section.

> 
> [1]: https://github.com/llvm/llvm-project/blob/de176d8c5496d6cf20e82aface98e102c593dbe2/lld/test/ELF/aarch64-thunk-pi.s#L112