Closed jbowler closed 2 months ago
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.
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".
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.
@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!)
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.
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.
Tested successfully on openSUSE Tumbleweed (with configure):
This fixes the bug https://github.com/pnggroup/libpng/issues/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:
https://github.com/pnggroup/libpng/pull/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