SECURITY: disable build of filter_neon.S on arm

[jbowler]

This fixes the bug #505
"libpng does not support PAC/BTI on aarch64 targets" which arises
because the build mechanisms (both cmake and configure) assemble
arm/filter_neon.S even though it ends up completely empty. The empty
file effectively poisons the so that the PAC/BTI support gets disabled.

The fix is minimal; it simply removes arm/filter_neon.S from the list of
sources included in the build. Note that this was already done in cmake
for MSVC - it's not clear whether this change was a partial fix for the
same issue.

The fix will cause attempts to use the assembler implementation to fail
at build time. As described in PR506:

#506

This should only cause problems with certain older GCC compilers and
only then if someone tries to build with the assembler optimization
enabled in which case the build probably had a security problem.

QUESTION: does the PAC/BTI security issue affect 32-bit ARM? If not
this change may might be an issue for someone given that filter_neon.S
would apparently be safe on 32-bit. Nevertheless this PR is safe
because it fails in a noisy way and is easy to undo.

TESTING: pull the changes then type "autoreconf" if using configure (not
required for cmake).

Signed-off-by: John Bowler jbowler@acm.org

[billatarm]

does the PAC/BTI security issue affect 32-bit ARM?

No aarch64 only. I'll test this later today and report back, looks right. Thanks.

[jbowler]

I'll make it so that nothing happens on 32-bit (i.e. the behavior in both cmake and configure builds with 32-bit won't change, only the 64-bit will drop the files.) That means there's less chance of someone being disrupted. BTW, does filter_neon.S even assemble with a 64-bit target? The code says ".arch armv7-a".

[billatarm]

I'll make it so that nothing happens on 32-bit (i.e. the behavior in both cmake and configure builds with 32-bit won't change, only the 64-bit will drop the files.) That means there's less chance of someone being disrupted. BTW, does filter_neon.S even assemble with a 64-bit target? The code says ".arch armv7-a".

No only works for aarch64. Ill keep an eye out for the updated patch.

[jbowler]

@billatarm here it is. The commit message has been updated with the information about the 32-bit support. So far as I can see this fix is minimal - all it does is remove filter_neon.S from the ARM 64-bit build and that file was always empty anyway!

I haven't been able to verify cmake builds; there's some problem with setting up a toolchain file with regard to the location of the zlib installtion (it's there in the host root but cmake does not find it.) The status of my tests (summarised in the check-in comment) is as follows:

CHOST	CFLAGS	configure	cmake	support	notes
armv7-linux-gnueabi	-	builds	[1]	none	CPU does not support NEON instructions
armv7a-linux-gnueabi	-	builds	[1]	none	Software FP: no NEON support
armv7a-hardfloat-linux-gnueabi	-	builds	[1]	none	NEON instructions not enabled in build
armv7a-hardfloat-linux-gnueabi	-mfpu=neon	builds	[2]	intrinsics
armv7a-hardfloat-linux-gnueabi	-mfpu=neon -DPNG_ARM_NEON_IMPLEMENTATION=2	[2]	TBD	assembler
aarch64-linux-gnu	-	builds	builds	intrinsics	filter_neon.S is not included in build
aarch64-linux-gnu	-DPNG_ARM_NEON_OPT=0	builds	TBD	none

[1] The CMakeLists.txt does not permit the intended and default way of enabling hardware optimisations by letting the compiler decide work, at least on ARM. It always sets PNG_ARM_NEON_OPT and it sets it to "2" (always on) by default on aarch64 and "0" (always off) on 32-bit arm. Since there is no way of leaving it undefined there is no way to check this change.

[2] Attempting to turn the NEON optimisations on with armv7a-hardfloat causes a build failure because filter_neon.S is not being passed -mfpu=neon, or at least it is failing to set either __ARM_NEON__ or __ARM_NEON, whereas these #defines are (both) set when filter_neon_intrinsics.c is compiled. This results in duplicate definition of the filter functions; this is not a new problem. In fact I think that's why the hardware optimisations were turned off; I seem to remember something about this. (There is no #issue though and it was pretty easy to debug!)

[billatarm]

Works for me:

autoreconf
export CFLAGS='-mbranch-protection=standard'
export LDFLAGS='-Wl,-zforce-bti -Wl,--fatal-warnings'
./configure
make -j4
readelf -n ./.libs/libpng16.so

Displaying notes found in: .note.gnu.property
  Owner                Data size 	Description
  GNU                  0x00000010	NT_GNU_PROPERTY_TYPE_0
      Properties: AArch64 feature: BTI, PAC

The Fedora package uses the autotools build system, so I did not look at the cmake side at all.

[jbowler]

I did not look at the cmake side at all.

I didn't know about readelf -n before. I've checked my cmake cross builds and aarch64 has BTI, PAC and 32-bit ARM doesn't build with cmake.

Note that I did not explicitly switch the features on; the only flags I'm setting are those in the table, the only relevant one is -mfpu=neon on 32-bit.

Likewise on the configure build of aarch64 I get BTI, PAC without any explicit request and with configure I can see the actual compile and link commands; neither "branch" nor "bti" appear in the output of make.:

On the configure builds various non-ARM code is compiled/assembled; this is because of some obvious errors in configure.ac which seem to have been copied and pasted.

Testing on amd64 to see what gets built arm/ and loongarch/ are empty (as they should be), intel/ is populated (as it should be) but mips/ and powerpc/ are both being built. On arm intel/ and loongarch/ are not built (correct) but mips/ and powerpc. are still happening. All the spurious files have BIT, PAC so I guess that's "ok" from the point of view of this bug. I am somewhat mystified that mips/filter_mmi_inline_assembly.o does have BTI, PAC set.

So to test I reverted this - back to libpng16:HEAD - and I can repro the lack of BTI, PAC in arm/filter_neon.o, also readelf -n on libpng16.so produces no output. To if @ctruta doesn't like this fix the problem can probably be fixed by using the same approach as in mips/filter_mmi_intrinsics.c.

@ctruta this is good to go; other issues have shown up but this provides a minimal fix to the security issue without breaking anything.

[ggardet]

Tested successfully on openSUSE Tumbleweed (with configure):

• aarch64: enabled PAC/BTI as expected
• armv7: builds with neon as expected