Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

selinux 3.8 build fails on x32 due to (erroneous?) asserts in matchpathcon.c #463

Open
kainz opened this issue Feb 21, 2025 · 3 comments · May be fixed by #464
Open

selinux 3.8 build fails on x32 due to (erroneous?) asserts in matchpathcon.c #463

kainz opened this issue Feb 21, 2025 · 3 comments · May be fixed by #464

Comments

@kainz
Copy link

kainz commented Feb 21, 2025

selinux 3.8 fails to build on libc-x32 due to a mismatch in the assumptions of sizeof(unsigned long), sizeof(uint32_t), and sizeof(__ino_t), as seen in https://github.com/SELinuxProject/selinux/blob/main/libselinux/src/matchpathcon.c#L270.

Given the hijinks involved in x32 with the 32-bit address space, 64-bit registers, and 64-bit kernel, making unsigned long not 32 bits, I'm actually not sure what the right thing to do here is. Maybe avoid bare unsigned long for mathpathcon_filesppec_add IAW sourceware's recommendations on x32/glibc at https://www.sourceware.org/glibc/wiki/x32?

While not necessarily relevant here, this does hit at least the debian x32 port:

cc -Wdate-time -D_FORTIFY_SOURCE=2 -D_LARGEFILE64_SOURCE -g -O2 -Werror=implicit-function-declaration -ffile-prefix-map=/<<PKGBUILDDIR>>=. -specs=/usr/share/dpkg/pie-compile.specs -fstack-protector-strong -Wformat -Werror=format-security -fcf-protection -fno-semantic-interposition -Wdate-time -D_FORTIFY_SOURCE=2 -Wall -Wextra -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I../include -D_GNU_SOURCE -DNO_ANDROID_BACKEND -DUSE_PCRE2 -DPCRE2_CODE_UNIT_WIDTH=8 -DHAVE_STRLCPY -DHAVE_REALLOCARRAY  -c -o matchpathcon.o matchpathcon.c
In file included from matchpathcon.c:1:
matchpathcon.c:270:1: error: static assertion failed: "inode size mismatch"
  270 | static_assert(sizeof(unsigned long) == sizeof(__ino_t), "inode size mismatch");
      | ^~~~~~~~~~~~~
make[3]: *** [Makefile:178: matchpathcon.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: Leaving directory '/<<PKGBUILDDIR>>/src'
make[2]: *** [Makefile:54: all] Error 1
make[2]: Leaving directory '/<<PKGBUILDDIR>>'
make[1]: *** [debian/rules:56: override_dh_auto_build] Error 2
make[1]: Leaving directory '/<<PKGBUILDDIR>>'
make: *** [debian/rules:50: binary-arch] Error 2

I suspect the 'right' thing to do is actually test on what x86_64-linux-gnux32/sys/types.h ends up declaring, but I don't know if that's sufficient for selinux. Hacking the static_asserts to do basically that seems to work, but I'm not in a spot where I can test that properly right now.
@kainz kainz changed the title selinux 3.8 bulid fails on x32 due to (erroneous?) asserts in matchpathcon.c selinux 3.8 build fails on x32 due to (erroneous?) asserts in matchpathcon.c Feb 21, 2025
@nabijaczleweli
Copy link

This looks like it was added in 9395cc0 and it conflates defined(_FILE_OFFSET_BITS) && _FILE_OFFSET_BITS == 64 && __BITS_PER_LONG < 64 with "ILP32 && ino_t without _FILE_OFFSET_BITS (<=> the kernel __ino_t) is 32 bits".

The check bundle is

    264 #if defined(_FILE_OFFSET_BITS) && _FILE_OFFSET_BITS == 64 && __BITS_PER_LONG < 64
    265 /* alias defined in the public header but we undefine it here */
    266 #undef matchpathcon_filespec_add
    267
    268 /* ABI backwards-compatible shim for non-LFS 32-bit systems */
    269
    270 static_assert(sizeof(unsigned long) == sizeof(__ino_t), "inode size mismatch");
    271 static_assert(sizeof(unsigned long) == sizeof(uint32_t), "inode size mismatch");
    272 static_assert(sizeof(ino_t) == sizeof(ino64_t), "inode size mismatch");
    273 static_assert(sizeof(ino64_t) == sizeof(uint64_t), "inode size mismatch");

which correctly identifies what it wants to do, but fails to use the correct condition and triggers on LFS ILP32 systems as well.

All the checks make sense (ulong=u32 is tautological, ino=ino64 confirms _F_O_B, ino64=u64 is tautological), ulong=__ino_t check ensures the ABI change is actually required, and fails on LFS ILP32 systems. To that extent the check is correct, except it wants to be lifted to the #if, because if ino_t=__ino_t=u64 then matchpathcon_filespec_add() had always taken an u64 in the first argument, so matchpathcon_filespec_add64() should not be produced.

nabijaczleweli added a commit to nabijaczleweli/selinux that referenced this issue Feb 22, 2025
@nabijaczleweli
Don't inject matchpathcon_filespec_add64() ifdef __x86_64__
edf4812 
As the code notes, it wants to add an
  /* ABI backwards-compatible shim for non-LFS 32-bit systems */
it tries to detect these with
  #if defined(_FILE_OFFSET_BITS) && _FILE_OFFSET_BITS == 64 && __BITS_PER_LONG < 64
which is correct with the added precondition that the ino_t /without/
-D_FILE_OFFSET_BITS=64 /was actually/ u32
(i.e. it conflates /all/ ILP32 systems into being non-LFS).

This is not the case on x32, for example, which is LFS; thus, the
  static_assert(sizeof(unsigned long) == sizeof(__ino_t), "inode size mismatch");
assertion fails (__ino_t is the "kernel ino_t" type,
which generally corresponds to the kernel's ulong, which is u64 on x32).

The correct spelling of the test for this is
  #if (...) && sizeof(__ino_t) == 4
but this is not statically solvable with the preprocessor.

Thus, we need to explcitly special-case this.
__x86_64__ indicates one of two ABIs (LP64 (amd64) or ILP32 (x32)),
both of which have ino_t=u64, and is the macro used for defining
__INO_T_TYPE in the system headers, so it's the best fit here.

Fixes: commit 9395cc0 ("Always build for LFS mode on 32-bit archs.")
Closes: SELinuxProject#463
Closes: Debian#1098481
@nabijaczleweli nabijaczleweli linked a pull request Feb 22, 2025 that will close this issue
Open
@noloader
Copy link

Regarding the commit, "Don't inject matchpathcon_filespec_add64() ifdef x86_64 (#463, Debian#1098481)"... I believe aarch64 will have a similar issue, so __x86_64__ is probably not quite correct.

Unfortunately, I don't remember all the details. But I recall __x86_64__ and __ILP32__ are combined slightly different with CGG and Clang. For GCC, __ILP32__ is defined in combination with __x86_64__ for x32. For Clang, __ILP32__ is always defined for a 32-bit platform (32-bit integers, longs and pointers). And that's where the problem with aarch32/aarch64 surfaces.

@nabijaczleweli
Copy link

nabijaczleweli commented Feb 23, 2025 •
edited
Loading

The correct spelling of this check is && sizeof(__ino_t) == 4 but we can't do this, so we must resort to listing exceptions. !__x86_64__ is correct here because it excludes both x86_64 ABIs: amd64 (already excluded by failing __BITS_PER_LONG < 64) and x32 (we care about this), both of which have __off_t=u64; checking __x86_64__ is correct, this (in combination with __ILP32__) is what /usr/include/x86_64-linux-gnux32/asm/bitsperlong.h and all the other glibc headers use.

I don't know what you mean by the second paragraph here. x32 is defined as __x86_64__ && __ILP32__. This must hold (and does) under all compilers. (I'd be surprised if __ILP32__ weren't always defined when building for ILP32 but, irrelevant here.)

arm64 is unaffected because it fails __BITS_PER_LONG < 64. This is the case for all LP64 ABIs. This exception list only concerns ILP32 ABIs with kernel __ino_t=u64, everything else is caught by the generic checks. idk if this is the case for anything except x32.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@kainz @noloader @nabijaczleweli