arch: add BR2_READELF_ARCH_NAME hidden config option

As shown recently by the firejail example, it is easy to miss that a package builds and installs binaries without actually cross-compiling
them: they are built for the host architecture instead of the target architecture.

This commit adds a small helper script, check-bin-arch, called from the main Makefile as a TARGET_FINALIZE_HOOKS, to verify that all ELF binaries have been built for the correct CPU architecture.

support/scripts/check-bin-arch new file +5 -0

new file mode 100755
index 0000000..cb29ded
@@ -0,0 +1,33 @@
+#!/bin/bash
+
+# This script looks at all files in the target filesystem, and for
+# those that are ELF files, verifies that they have been built for the
+# correct architecture.

support/scripts/check-bin-arch new file +5 -0

+	exitcode=1
+    fi
+done
+
+exit ${exitcode}

Signature · 1 line

2.7.4

On 2017-03-12 18:49 +0100, Thomas Petazzoni spake thusly:

Quoted 1 line

+for f in $(find ${TARGET_DIR}/{usr/,}{bin,lib,sbin} -type f) ; do

support/scripts/check-bin-arch line 19

Quoted 3 lines

support/scripts/check-bin-arch new file +3 -0

+    # Skip non-ELF files
+    if ! file -b ${f} | grep -q "ELF " ; then
+	continue

You have a mix of space-and-tab indetation. Please use only one or the other (I favour spaces, but I'm happy with tabs as long as it is consistent).

support/scripts/check-bin-arch line 27

Quoted 8 lines

support/scripts/check-bin-arch new file +8 -0

+    fi
+
+    # Get architecture using readelf
+    farchname=$(${TARGET_CROSS}readelf -h ${f} | \
+		       grep '^  Machine:' | \
+		       sed 's/^  Machine: *\(.*\)/\1/')
+
+    if test "${farchname}" != "${READELF_ARCH_NAME}" ; then

[0] because we could then point to the offending package:

        pkg="$( sed -r -e "\:^([^,]+),${f}$:!d; s//\1/;" "${BUILD_DIR}/packages-file-list.txt" )"

Of course, do not forget to pass BUILD_DIR when calling the script. ;-)

support/scripts/check-bin-arch line 28

Quoted 1 line

support/scripts/check-bin-arch new file +1 -0

+	echo "ERROR: ${f} architecture is '${farchname}', should be '${READELF_ARCH_NAME}'"

    printf 'ERROR: %s (from %s) architecture is %s, should be %s\n' \
        "${f}" "${pkg}" "${farchname}" "${READELF_ARCH_NAME}"

Regards,
Yann E. MORIN.

Quoted 8 lines

support/scripts/check-bin-arch new file +5 -0

+	exitcode=1
+    fi
+done
+
+exit ${exitcode}

Signature · 1 line

2.7.4

Signature · 10 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

Thomas, All,

On 2017-03-12 19:05 +0100, Yann E. MORIN spake thusly:

Quoted 2 lines

On 2017-03-12 18:49 +0100, Thomas Petazzoni spake thusly:

Quoted 1 line

+for f in $(find ${TARGET_DIR}/{usr/,}{bin,lib,sbin} -type f) ; do

So, as I said earlier:
- cd in TARGET_DIR first;
- exclude .lib/firmware and /usr/lib/firmware

    cd "${TARGET_DIR}"
    find ./{usr/,}{bin,lib,sbin} \
        -type f \( \( -path './lib/firmware/*' -o -path './usr/lib/firmware*' \) \ 
                    -prune -o -print \)

Quoted 9 lines

Thomas, All,

On 2017-03-12 19:05 +0100, Yann E. MORIN spake thusly:

Quoted 2 lines

On 2017-03-12 18:49 +0100, Thomas Petazzoni spake thusly:

Quoted 1 line

+for f in $(find ${TARGET_DIR}/{usr/,}{bin,lib,sbin} -type f) ; do

So, as I said earlier:
- cd in TARGET_DIR first;
- exclude .lib/firmware and /usr/lib/firmware

Quoted 4 lines

    cd "${TARGET_DIR}"
    find ./{usr/,}{bin,lib,sbin} \
        -type f \( \( -path './lib/firmware/*' -o -path './usr/lib/firmware*' \) \ 
                    -prune -o -print \)

As I wrote in the older thread, there are also binaries in /usr/share.

That
basically leaves nothing except /etc, and a specific exclusion is anyway still needed for /lib/firmware, so I would just do "find .".

Regards,
Arnout

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Arnout, All,

On 2017-03-12 20:43 +0100, Arnout Vandecappelle spake thusly:

Quoted 16 lines · Yann E. MORIN

Quoted 13 lines

Thomas, All,

On 2017-03-12 19:05 +0100, Yann E. MORIN spake thusly:

Quoted 2 lines

On 2017-03-12 18:49 +0100, Thomas Petazzoni spake thusly:

Quoted 1 line

+for f in $(find ${TARGET_DIR}/{usr/,}{bin,lib,sbin} -type f) ; do

So, as I said earlier:
- cd in TARGET_DIR first;
- exclude .lib/firmware and /usr/lib/firmware

    cd "${TARGET_DIR}"
    find ./{usr/,}{bin,lib,sbin} \
        -type f \( \( -path './lib/firmware/*' -o -path './usr/lib/firmware*' \) \ 
                    -prune -o -print \)

Quoted 1 line

As I wrote in the older thread, there are also binaries in /usr/share.

What kind of binaries are in /usr/share? I've looked at my system (Ubuntu 16.04, x86_64), and none of the 139615 files in there is an ELF file.

Quoted 4 lines

That
basically leaves nothing except /etc, and a specific exclusion is anyway still needed for /lib/firmware, so I would just do "find .".

To be clear, you would do 'find .' instead of 'find ./{usr/,}{bin,lib,sbin}', but still do the exclusion as I suggested above, right?

I would be OK with that. Afterall, if one is smart enough to put an ELF file for a co-proc somewhere else than in /lib/firmware, too bad for them.

Regards,
Yann E. MORIN.

Quoted 21 lines

Arnout, All,

On 2017-03-12 20:43 +0100, Arnout Vandecappelle spake thusly:

Quoted 17 lines · Yann E. MORIN

Quoted 5 lines

Thomas, All,

On 2017-03-12 19:05 +0100, Yann E. MORIN spake thusly:

Quoted 2 lines

On 2017-03-12 18:49 +0100, Thomas Petazzoni spake thusly:

Quoted 1 line

+for f in $(find ${TARGET_DIR}/{usr/,}{bin,lib,sbin} -type f) ; do

Quoted 8 lines

So, as I said earlier:
- cd in TARGET_DIR first;
- exclude .lib/firmware and /usr/lib/firmware

    cd "${TARGET_DIR}"
    find ./{usr/,}{bin,lib,sbin} \
        -type f \( \( -path './lib/firmware/*' -o -path './usr/lib/firmware*' \) \ 
                    -prune -o -print \)

As I wrote in the older thread, there are also binaries in /usr/share.

Quoted 3 lines

What kind of binaries are in /usr/share? I've looked at my system (Ubuntu 16.04, x86_64), and none of the 139615 files in there is an ELF file.

I found /usr/share/bash-completion/helpers/gst-completion-helper-1.0 in one of
my build results. Wolfgang (now in Cc) also found something, I'm not sure if it was the same thing or something different.

Quoted 13 lines

Quoted 3 lines

That
basically leaves nothing except /etc, and a specific exclusion is anyway still needed for /lib/firmware, so I would just do "find .".

To be clear, you would do 'find .' instead of 'find ./{usr/,}{bin,lib,sbin}', but still do the exclusion as I suggested above, right?

I would be OK with that. Afterall, if one is smart enough to put an ELF file for a co-proc somewhere else than in /lib/firmware, too bad for them.

Regards,
Yann E. MORIN.

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Makefile +1 -0

+
 $(TARGETS_ROOTFS): target-finalize
 
 target-finalize: $(PACKAGES)

support/scripts/check-bin-arch new file +0 -0

new file mode 100755
index 0000000..cb29ded

support/scripts/check-bin-arch new file +8 -0

+
+    if test "${farchname}" != "${READELF_ARCH_NAME}" ; then
+	echo "ERROR: ${f} architecture is '${farchname}', should be '${READELF_ARCH_NAME}'"
+	exitcode=1
+    fi
+done
+
+exit ${exitcode}

[snip]

Makefile endef · line 654

Quoted 3 lines

Makefile +3 -0

+READELF_ARCH_NAME = $(call qstrip,$(BR2_READELF_ARCH_NAME))
+
+ifneq ($(READELF_ARCH_NAME),)

We set it for all arches, so this is never empty. Except when we forget to set
it, but in that case we probably want this thing to be executed so we notice.

Makefile endef · line 660

Quoted 6 lines

Makefile +6 -0

+define CHECK_BIN_ARCH
+	support/scripts/check-bin-arch $(TARGET_DIR) \
+		$(TARGET_CROSS) "$(READELF_ARCH_NAME)"
+endef
+TARGET_FINALIZE_HOOKS += CHECK_BIN_ARCH
+endif

It would be nice to do it immediately after package install, so it becomes a
package error rather than some post-build error that is more difficult to localise. Of course, with Yann's suggestion it would still report which package was the culprit, but I don't think it would show up in the autobuild results under the right package.

To avoid too much overhead rechecking files all the time, it could be done as
part of step_pkg_size_end which iterates over the files installed in target for that particular package. It would make the code a little more complicated, though, and perhaps also slower.

Quoted 1 line

+ farchname=$(${TARGET_CROSS}readelf -h ${f} | \

support/scripts/check-bin-arch line 25

Quoted 2 lines

support/scripts/check-bin-arch new file +2 -0

+		       grep '^  Machine:' | \
+		       sed 's/^  Machine: *\(.*\)/\1/')

Why not the simpler sed -n '/^ Machine: *\(.*\)/s//\1/p' instead of the extra
grep?

support/scripts/check-bin-arch line 18

Quoted 7 lines

support/scripts/check-bin-arch new file +6 -0

+# In order to avoid matching firmware files that could have the ELF
+# format, but for other architectures, we only look in bin, lib, sbin,
+# usr/bin, usr/lib and usr/sbin
+for f in $(find ${TARGET_DIR}/{usr/,}{bin,lib,sbin} -type f) ; do
+    # Skip non-ELF files
+    if ! file -b ${f} | grep -q "ELF " ; then

As observed by Wolfgang in the context of rpath sanitisation, file is pretty
inefficient. We came to the conclusion that the most efficient is just checking the first 4 bytes (0x7f 0x45 0x4c 0x46). Since it is also needed for the rpath sanitisation, perhaps we should have a common script, or even compiled code, to do this check.

Note however that just checking the first 4 bytes gives a bunch of false
positives, so we'd have to ignore errors from readelf as well.

Quoted 1 line

+ifneq ($(READELF_ARCH_NAME),)

We set it for all arches, so this is never empty. Except when we forget to set
it, but in that case we probably want this thing to be executed so we notice.

It would be nice to do it immediately after package install, so it becomes a
package error rather than some post-build error that is more difficult to localise. Of course, with Yann's suggestion it would still report which package was the culprit, but I don't think it would show up in the autobuild results under the right package.

To avoid too much overhead rechecking files all the time, it could be done as
part of step_pkg_size_end which iterates over the files installed in target for that particular package. It would make the code a little more complicated, though, and perhaps also slower.

Top-level Makefile already exports TARGET_DIR, so no need to pass it as an
argument. And it's not as if this script is meant to be ran outside of the build.

This could also be grepped out of BR2_CONFIG, like mkusers does it.

As observed by Wolfgang in the context of rpath sanitisation, file is pretty
inefficient. We came to the conclusion that the most efficient is just checking the first 4 bytes (0x7f 0x45 0x4c 0x46). Since it is also needed for the rpath sanitisation, perhaps we should have a common script, or even compiled code, to do this check.

Why not the simpler sed -n '/^ Machine: *\(.*\)/s//\1/p' instead of the extra
grep?