Re: [PATCH] Always build for LFS mode on 32-bit archs.

[Steve Langasek <steve.langasek@canonical.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2709 bytes --]

Hi Kees,

On Sat, Feb 17, 2024 at 01:54:14PM -0800, Kees Cook wrote:
> On Thu, Feb 15, 2024 at 04:35:24PM -0800, Steve Langasek wrote:
> > index a0948853..78953870 100644
> > --- a/libselinux/include/selinux/selinux.h
> > +++ b/libselinux/include/selinux/selinux.h
> > @@ -1,6 +1,7 @@
> >  #ifndef _SELINUX_H_
> >  #define _SELINUX_H_
> >  
> > +#include <stdint.h>
> >  #include <sys/types.h>
> >  #include <stdarg.h>
> >  
> > @@ -521,7 +522,11 @@ extern int matchpathcon_index(const char *path,
> >     with the same inode (e.g. due to multiple hard links).  If so, then
> >     use the latter of the two specifications based on their order in the 
> >     file contexts configuration.  Return the used specification index. */
> > -extern int matchpathcon_filespec_add(ino_t ino, int specind, const char *file);
> > +typedef uint64_t libselinux_ino_t;
> > +#if _FILE_OFFSET_BITS == 64 && __BITS_PER_LONG < 64
> > +#define matchpathcon_filespec_add matchpathcon_filespec_add64
> > +#endif
> > +extern int matchpathcon_filespec_add(libselinux_ino_t ino, int specind, const char *file);

> What's the ABI goal here? I think the type is wrong -- doesn't this need
> to be uint32_t not a uint64_t for the wrapper?

Thanks for the review!  The intent is that there are 3 cases:

- on 64-bit archs, matchpathcon_filespec_add64() is not defined because the
  ino_t passed to matchpathcon_filespec_add() is already 64-bit
- on 32-bit archs when LFS support is defined, references to
  matchpathcon_filespec_add() will be rewritten to
  matchpathcon_filespec_add64() because the ino_t argument passed to _add()
  is* 32-bit
- on 32-bit archs when LFS support is not defined, the existing
  matchpathcon_filespec_add() entry point should be used as-is.

And in the process of articulating this, I've come to recognize there was a
bug here, because in the case where our arch is 32-bit and our caller is NOT
being invoked with LFS build flags, we are exposing the previous
matchpathcon_filespec_add() but have wrongly modified the prototype here to
take a 64-bit ino_t.

I believe the attached revised patch corrects this.

-- 
Steve Langasek                   Give me a lever long enough and a Free OS
Debian Developer                   to set it on, and I can move the world.
Ubuntu Developer                                   https://www.debian.org/
slangasek@ubuntu.com                                     vorlon@debian.org

* may be.  There are some 32-bit archs defined in glibc/Linux that do not
  treat 32-bit time_t as an option and AIUI thus also have 64-bit ino_t; in
  which case the additional entry point is unnecessary but also of
  low impact.

[-- Attachment #1.2: 0001-Always-build-for-LFS-mode-on-32-bit-archs.patch --]
[-- Type: text/x-diff, Size: 4794 bytes --]

From b23c9044b542b8807f57f4691f4bd1149cbee04c Mon Sep 17 00:00:00 2001
From: Steve Langasek <steve.langasek@canonical.com>
Date: Thu, 15 Feb 2024 15:22:45 -0800
Subject: [PATCH] Always build for LFS mode on 32-bit archs.

Maintains the type signature of the existing matchpathcon_filespec_add()
entry point on 32-bit archs but maps the API to a new
matchpathcon_filespec_add64() entry point that takes a 64-bit ino_t argument
instead.

Software on 32-bit Linux ports which historically use a 32-bit time_t (thus
affected by the y2038 problem) have, as a precondition of migrating to
64-bit time_t, that they also migrate to large filesystem support because
glibc does not provide entry points for the cross-product of
(LFS: yes, LFS: no) x (time_t: 32, time_t: 64).

In order to support smooth migration of such operating systems from 32-bit
time_t to 64-bit time_t, it is useful for libselinux to:

- provide entry points on 32-bit systems for both LFS and non-LFS variants
  of the API (as glibc itself does)
- use LFS internally for all filesystem calls (just in case)
- map the API call to the correct implementation based on the build
  environment of the caller.

Signed-off-by: Steve Langasek <steve.langasek@canonical.com>
---
 libselinux/include/selinux/selinux.h | 11 ++++++++++-
 libselinux/src/Makefile              |  1 +
 libselinux/src/libselinux.map        |  1 +
 libselinux/src/matchpathcon.c        | 15 ++++++++++++++-
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/libselinux/include/selinux/selinux.h b/libselinux/include/selinux/selinux.h
index a0948853..040629c3 100644
--- a/libselinux/include/selinux/selinux.h
+++ b/libselinux/include/selinux/selinux.h
@@ -1,6 +1,7 @@
 #ifndef _SELINUX_H_
 #define _SELINUX_H_
 
+#include <stdint.h>
 #include <sys/types.h>
 #include <stdarg.h>
 
@@ -521,7 +522,15 @@ extern int matchpathcon_index(const char *path,
    with the same inode (e.g. due to multiple hard links).  If so, then
    use the latter of the two specifications based on their order in the 
    file contexts configuration.  Return the used specification index. */
-extern int matchpathcon_filespec_add(ino_t ino, int specind, const char *file);
+#if _FILE_OFFSET_BITS == 64
+typedef uint64_t libselinux_ino_t;
+#if __BITS_PER_LONG < 64
+#define matchpathcon_filespec_add matchpathcon_filespec_add64
+#endif
+#else
+typedef uint32_t libselinux_ino_t;
+#endif
+extern int matchpathcon_filespec_add(libselinux_ino_t ino, int specind, const char *file);
 
 /* Destroy any inode associations that have been added, e.g. to restart
    for a new filesystem. */
diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
index d3b981fc..267291aa 100644
--- a/libselinux/src/Makefile
+++ b/libselinux/src/Makefile
@@ -87,6 +87,7 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k -Wformat-security -Winit-self -Wmissi
           -fstack-protector-all --param=ssp-buffer-size=4 -fexceptions \
           -fasynchronous-unwind-tables -fdiagnostics-show-option \
           -Werror -Wno-aggregate-return \
+          -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 \
           $(EXTRA_CFLAGS)
 
 LD_SONAME_FLAGS=-soname,$(LIBSO),--version-script=libselinux.map,-z,defs,-z,relro
diff --git a/libselinux/src/libselinux.map b/libselinux/src/libselinux.map
index 5e00f45b..88c9b3e5 100644
--- a/libselinux/src/libselinux.map
+++ b/libselinux/src/libselinux.map
@@ -251,4 +251,5 @@ LIBSELINUX_3.5 {
   global:
     getpidprevcon;
     getpidprevcon_raw;
+    matchpathcon_filespec_add64;
 } LIBSELINUX_3.4;
diff --git a/libselinux/src/matchpathcon.c b/libselinux/src/matchpathcon.c
index e44734c3..189e00fb 100644
--- a/libselinux/src/matchpathcon.c
+++ b/libselinux/src/matchpathcon.c
@@ -195,7 +195,8 @@ static file_spec_t *fl_head;
  * then use the specification that occurs later in the
  * specification array.
  */
-int matchpathcon_filespec_add(ino_t ino, int specind, const char *file)
+int matchpathcon_filespec_add(libselinux_ino_t ino, int specind,
+                              const char *file)
 {
 	file_spec_t *prevfl, *fl;
 	int h, ret;
@@ -261,6 +262,18 @@ int matchpathcon_filespec_add(ino_t ino, int specind, const char *file)
 	return -1;
 }
 
+#if _FILE_OFFSET_BITS == 64 && __BITS_PER_LONG < 64
+/* alias defined in the public header but we undefine it here */
+#undef matchpathcon_filespec_add
+
+/* ABI backwards-compatible shim for non-LFS 32-bit systems */
+int matchpathcon_filespec_add(unsigned long ino, int specind,
+                              const char *file)
+{
+	return matchpathcon_filespec_add64(ino, specind, file);
+}
+#endif
+
 /*
  * Evaluate the association hash table distribution.
  */
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]