#367 aarch64 assembler files are missing PAC/BTI flags

[Guillaume GARDET]

On aarch64/arm64, when we build with gcc13 and -mbranch-protection=standard to enable Pointer Authentication (PAC) and Branch Target Identification (BTI), the final link disable them, because some assembler code do not support PAC and BTI.

This is a matter to add paciasp/autiasp in start/end of functions for PAC and BTI C (or better hint #34) as landing pad on branches for BTI.

More information on :
* PAC: https://developer.arm.com/documentation/102433/0100/Return-oriented-programming
* BTI: https://developer.arm.com/documentation/102433/0100/Jump-oriented-programming

[Thomas Orgis]

Wich exact version are you referring to and which assembler function, specifically? I hoped to have that issue avoided via this change noted in NEWS for 1.32.4:

• libmpg123:
  -- Avoid indirect branches into the assembly routines by using C wrappers
  also for dct36, relieving us of the need to care for bti / endbr
  instructions for control flow integrity.

[Guillaume GARDET]

This is with version 1.32.4.

Maybe related, I have this warning:

[   32s] src/libmpg123/layer3.c:1491:12: warning: 'INT123_dct36_neon64_wrap' defined but not used [-Wunused-function]
[   32s]  1491 | DCT36_WRAP(INT123_dct36_neon64)
[   32s]       |            ^~~~~~~~~~~~~~~~~~~
[   32s] src/libmpg123/layer3.c:1467:13: note: in definition of macro 'DCT36_WRAP'
[   32s]  1467 | static void asmfunc ## _wrap(real *inbuf,real *o1,real *o2,const real *wintab,real *tsbuf) \
[   32s]       |             ^~~~~~~

[Thomas Orgis]

So you have OPT_NEON64 defined so that the wrapper function is created, but it's not assigned to fr->cpu.opts.the_dct36 later on in layer3.c?

Oh, of course. The case without OPT_MULTI didn't use the wrappers yet. Please check current https://mpg123.org/snapshot (or svn trunk).

[Thomas Orgis]

Wait, this is warped. We should not need the wrappers without OPT_MULTI. There should be no indirect banches. On other platforms, this issue does not arise as we do not call into asm via function pointers.

Can you be more specific as to which assembly function is to blame? Where is the indirect branch? Or is the linker just doing this blindly?

[Thomas Orgis]

You're building --with-cpu=aarch64? Choosing plain neon64 would not use a wrapper, but …

  neon64)
    ADD_CPPFLAGS="$ADD_CPPFLAGS -DOPT_NEON64 -DREAL_IS_FLOAT"
    more_sources="$s_neon64"
  ;;
  aarch64)
    ADD_CPPFLAGS="$ADD_CPPFLAGS -DOPT_MULTI -DOPT_GENERIC -DOPT_GENERIC_DITHER -DOPT_NEON64 -DREAL_IS_FLOAT"
    more_sources="$s_neon64 $s_dither $s_arm_multi"
  ;;

So this is a multi build that should use the wrapper.

And … oh … dear … what stupid thing: I simply didn't complete the wrapper usage in INT123_dct36_choose(). This also triggers for x86-64 if you care to build with ./configure --enable-nagging.

I fixed that omission … but right now see breakge because of stuff in getcpuflags_arm.c that might be just because of my cross toolchain.

Can you confirm that current svn trunk (soon to be snapshot) is fine?

[Thomas Orgis]

I settled the build issue (std=c99 surfaced missing defines in the getcpuflags code for siglongjmp stuff.

I'll close this, assuming things are fine now after the correction to use the wrappers.

[Guillaume GARDET]

The warning is gone with 1.32.5 but the original issue is still open since *.S files are not fixed.

[Thomas Orgis]

The point is that there are no indirect branches into the asm code anymore and hence no landing pads needed. At least that seems to fix things on x86. Is that different on arm? Is there an actual issue left, or do you just want those instructions there for themselves?

sent from mobile device, trustworthy or not

[Thomas Orgis]

Closing for now … I built using clang and branch protection in termux on an Android aarch64 device. It only complained about some prototype-less functions that I'll also fix, but nothing about it still wanting landing pads for the wrapped assembly.

[Bill Roberts]

@sobukus, reviving this. So AFAICT the assembly routines for aarch64 do not store the LR (x30) to the stack. So they don't need pac instructions. However, they will need bti instructions, specifically 'bti c' at the entry points. Here is a link to a PR I did for Pixman: https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/105/diffs

But the steps are similair:
For each assembly file:
- ensure the gnu notes section is added (#include usually the easiest)

This will mark that BTI is enabled and the resulting elf file will be loaded and mapped with PROT_BTI so it's enforced.

If you wanna spin a patch i'd be happy to test, or be patient and I may be able to get to it.

[Thomas Orgis]

I was busy with this BTI stuff before and figured that, at least for x86-64, all is well by just ensuring that the assembly is not called via indirect branching (aka function pointers).

When resarching the issue back then, I came to the conclusion that wrapping all calls to the assembly into C functions that then get all compiler ornamentation for BTI and whatnot enables the compiler hardening without having to mess with the assembly files yet again (like with executable stack before).

Can you give me clear instructions on how to verify that the current state with the wrapper functions and no function pointers to the assembly routines themselves still makes the compiler unhappy? I tried building with clang in termux, as that was the easiest aarch64 setup available to me right now. Is this complaint, disabling of branch protection, specific to gcc? Maybe it is actually a compiler bug? It should be able to just enable the branch protection and it should work.

Except if something is different on ARM that the avoidance of function pointers doesn't avoid the indirect branches/jumps.

Sorry if that looks stubborn … but I thought I solved that problem long ago and don't fancy messing with the assembly for compiler-specific stuff again and again.

[Thomas Orgis]

(With instructiuons, I mean including tool calls to verify if a binary has branch protection enabled or not … my memory is rusty….)

[Thomas Orgis]

According to https://developer.arm.com/documentation/101754/0616/armclang-Reference/armclang-Command-line-Options/-mbranch-protection this build I did on my ARMv8.2 phone should enable BTI:

CFLAGS='-Os -mbranch-protection=bti -march=armv8-a" ./configure --with-optimization=0 --enable-nagging
make

I don't see any issue being reported. It do see linker warnings at runtime regarding unused DT entry that may be related to the alluded no-ops on this older CPU ... those warnings are only in libout123 and libsyn123, as well the mpg123 binary. This might be an indication that libmpg123 itself misses those, meaning it does not have BTI enabled.

But why does the compiler not complain? Can I get some more definite diagnostics?