Linux Test Project
 help / color / mirror / Atom feed
* [LTP] [PATCH v4 0/2] Add listxattr04 test reproducer
@ 2025-07-22  6:55 Andrea Cervesato
  2025-07-22  6:55 ` [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility Andrea Cervesato
  2025-07-22  6:55 ` [LTP] [PATCH v4 2/2] Add listxattr04 reproducer Andrea Cervesato
  0 siblings, 2 replies; 20+ messages in thread
From: Andrea Cervesato @ 2025-07-22  6:55 UTC (permalink / raw)
  To: ltp

Reproducer for https://lore.kernel.org/linux-fsdevel/m1wm9qund4.fsf@gmail.com/T/

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
Changes in v4:
- check if SELinux is enabled by looking at the mount point
- Link to v3: https://lore.kernel.org/r/20250709-xattr_bug_repr-v3-0-379c2c291bb7@suse.com

Changes in v3:
- add tst_selinux_enabled() utility
- Link to v2: https://lore.kernel.org/r/20250703-xattr_bug_repr-v2-1-154e9afe2463@suse.com

Changes in v2:
- only check if SELinux is up and running
- Link to v1: https://lore.kernel.org/r/20250703-xattr_bug_repr-v1-1-5dcf5dde8b61@suse.com

---
Andrea Cervesato (2):
      core: add tst_selinux_enabled() utility
      Add listxattr04 reproducer

 include/tst_security.h                            |   1 +
 lib/tst_security.c                                |  15 ++-
 testcases/kernel/syscalls/listxattr/.gitignore    |   1 +
 testcases/kernel/syscalls/listxattr/Makefile      |   2 +
 testcases/kernel/syscalls/listxattr/listxattr04.c | 108 ++++++++++++++++++++++
 5 files changed, 126 insertions(+), 1 deletion(-)
---
base-commit: 81ba405dfbd9a36284ec83daa3d8938c1558340f
change-id: 20250702-xattr_bug_repr-5873b84792fb

Best regards,
-- 
Andrea Cervesato <andrea.cervesato@suse.com>


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22  6:55 [LTP] [PATCH v4 0/2] Add listxattr04 test reproducer Andrea Cervesato
@ 2025-07-22  6:55 ` Andrea Cervesato
  2025-07-22 10:51   ` Petr Vorel
                     ` (2 more replies)
  2025-07-22  6:55 ` [LTP] [PATCH v4 2/2] Add listxattr04 reproducer Andrea Cervesato
  1 sibling, 3 replies; 20+ messages in thread
From: Andrea Cervesato @ 2025-07-22  6:55 UTC (permalink / raw)
  To: ltp

From: Andrea Cervesato <andrea.cervesato@suse.com>

Add tst_selinux_enabled() utility in tst_security.h in order to verify
if SELinux is currently up and running in the system.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 include/tst_security.h |  1 +
 lib/tst_security.c     | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/tst_security.h b/include/tst_security.h
index 5d91f8a98f104b0cafaaf2046bc0ceec06870606..cb5490a896f027245064abebb9d7c36270fd2e8a 100644
--- a/include/tst_security.h
+++ b/include/tst_security.h
@@ -14,5 +14,6 @@ int tst_fips_enabled(void);
 int tst_lockdown_enabled(void);
 int tst_secureboot_enabled(void);
 int tst_selinux_enforcing(void);
+int tst_selinux_enabled(void);
 
 #endif /* TST_SECURITY_H__ */
diff --git a/lib/tst_security.c b/lib/tst_security.c
index 7d929fafe729058f55b921bf5cf7806b253496e0..f4669c60fbcafeddcab23835ee8c568a4aab46c3 100644
--- a/lib/tst_security.c
+++ b/lib/tst_security.c
@@ -7,7 +7,8 @@
 
 #define PATH_FIPS	"/proc/sys/crypto/fips_enabled"
 #define PATH_LOCKDOWN	"/sys/kernel/security/lockdown"
-#define SELINUX_STATUS_PATH "/sys/fs/selinux/enforce"
+#define SELINUX_PATH "/sys/fs/selinux"
+#define SELINUX_STATUS_PATH (SELINUX_PATH "/enforce")
 
 #if defined(__powerpc64__) || defined(__ppc64__)
 # define SECUREBOOT_VAR "/proc/device-tree/ibm,secure-boot"
@@ -102,6 +103,18 @@ int tst_secureboot_enabled(void)
 	return data[VAR_DATA_SIZE - 1];
 }
 
+int tst_selinux_enabled(void)
+{
+	int res = 0;
+
+	if (tst_is_mounted(SELINUX_PATH))
+		res = 1;
+
+	tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");
+
+	return res;
+}
+
 int tst_selinux_enforcing(void)
 {
 	int res = 0;

-- 
2.50.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [LTP] [PATCH v4 2/2] Add listxattr04 reproducer
  2025-07-22  6:55 [LTP] [PATCH v4 0/2] Add listxattr04 test reproducer Andrea Cervesato
  2025-07-22  6:55 ` [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility Andrea Cervesato
@ 2025-07-22  6:55 ` Andrea Cervesato
  2025-07-22 14:11   ` Petr Vorel
  1 sibling, 1 reply; 20+ messages in thread
From: Andrea Cervesato @ 2025-07-22  6:55 UTC (permalink / raw)
  To: ltp

From: Andrea Cervesato <andrea.cervesato@suse.com>

Test reproducer for a bug introduced in 8b0ba61df5a1 ("fs/xattr.c: fix
simple_xattr_list to always include security.* xattrs").

Bug can be reproduced when SELinux and ACL are activated on inodes as
following:

    $ touch testfile
    $ setfacl -m u:myuser:rwx testfile
    $ getfattr -dm. /tmp/testfile
    Segmentation fault (core dumped)

The reason why this happens is that simple_xattr_list() always includes
security.* xattrs without resetting error flag after
security_inode_listsecurity(). This results into an incorrect length of the
returned xattr name if POSIX ACL is also applied on the inode.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 testcases/kernel/syscalls/listxattr/.gitignore    |   1 +
 testcases/kernel/syscalls/listxattr/Makefile      |   2 +
 testcases/kernel/syscalls/listxattr/listxattr04.c | 108 ++++++++++++++++++++++
 3 files changed, 111 insertions(+)

diff --git a/testcases/kernel/syscalls/listxattr/.gitignore b/testcases/kernel/syscalls/listxattr/.gitignore
index be0675a6df0080d176d53d70194442bbc9ed376c..0d672b6ea5eec03aab37ee89316c56e24356c1d9 100644
--- a/testcases/kernel/syscalls/listxattr/.gitignore
+++ b/testcases/kernel/syscalls/listxattr/.gitignore
@@ -1,3 +1,4 @@
 /listxattr01
 /listxattr02
 /listxattr03
+/listxattr04
diff --git a/testcases/kernel/syscalls/listxattr/Makefile b/testcases/kernel/syscalls/listxattr/Makefile
index c2f84b1590fc24a7a98f890ea7706771d944aa79..e96bb3fa4c2c6b14b8d2bc8fd4c475e4789d72fe 100644
--- a/testcases/kernel/syscalls/listxattr/Makefile
+++ b/testcases/kernel/syscalls/listxattr/Makefile
@@ -6,4 +6,6 @@ top_srcdir		?= ../../../..
 
 include $(top_srcdir)/include/mk/testcases.mk
 
+listxattr04: LDLIBS	+= $(ACL_LIBS)
+
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/listxattr/listxattr04.c b/testcases/kernel/syscalls/listxattr/listxattr04.c
new file mode 100644
index 0000000000000000000000000000000000000000..473ed45b5c2da8ff8e49c513eeb82158ec2dc066
--- /dev/null
+++ b/testcases/kernel/syscalls/listxattr/listxattr04.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2025 Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * Test reproducer for a bug introduced in 8b0ba61df5a1 ("fs/xattr.c: fix
+ * simple_xattr_list to always include security.* xattrs").
+ *
+ * Bug can be reproduced when SELinux and ACL are activated on inodes as
+ * following:
+ *
+ *     $ touch testfile
+ *     $ setfacl -m u:myuser:rwx testfile
+ *     $ getfattr -dm. /tmp/testfile
+ *     Segmentation fault (core dumped)
+ *
+ * The reason why this happens is that simple_xattr_list() always includes
+ * security.* xattrs without resetting error flag after
+ * security_inode_listsecurity(). This results into an incorrect length of the
+ * returned xattr name if POSIX ACL is also applied on the inode.
+ */
+
+#include "config.h"
+#include "tst_test.h"
+
+#if defined(HAVE_SYS_XATTR_H) && defined(HAVE_LIBACL)
+
+#include <pwd.h>
+#include <sys/acl.h>
+#include <sys/xattr.h>
+
+#define ACL_PERM        "u::rw-,u:root:rwx,g::r--,o::r--,m::rwx"
+#define TEST_FILE       "test.bin"
+
+static acl_t acl;
+
+static void verify_xattr(const int size)
+{
+	char buf[size];
+
+	memset(buf, 0, sizeof(buf));
+
+	if (listxattr(TEST_FILE, buf, size) == -1) {
+		if (errno != ERANGE)
+			tst_brk(TBROK | TERRNO, "listxattr() error");
+
+		tst_res(TFAIL, "listxattr() failed to read attributes length: ERANGE");
+		return;
+	}
+
+	tst_res(TPASS, "listxattr() correctly read attributes length");
+}
+
+static void run(void)
+{
+	int size;
+
+	size = listxattr(TEST_FILE, NULL, 0);
+	if (size == -1)
+		tst_brk(TBROK | TERRNO, "listxattr() error");
+
+	verify_xattr(size);
+}
+
+static void setup(void)
+{
+	int res;
+
+	if (!tst_selinux_enabled())
+		tst_brk(TCONF, "SELinux is not enabled");
+
+	SAFE_TOUCH(TEST_FILE, 0644, NULL);
+
+	acl = acl_from_text(ACL_PERM);
+	if (!acl)
+		tst_brk(TBROK | TERRNO, "acl_from_text() failed");
+
+	res = acl_set_file(TEST_FILE, ACL_TYPE_ACCESS, acl);
+	if (res == -1) {
+		if (errno == EOPNOTSUPP)
+			tst_brk(TCONF | TERRNO, "acl_set_file()");
+
+		tst_brk(TBROK | TERRNO, "acl_set_file(%s) failed", TEST_FILE);
+	}
+}
+
+static void cleanup(void)
+{
+	if (acl)
+		acl_free(acl);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "800d0b9b6a8b"},
+		{}
+	}
+};
+
+#else /* HAVE_SYS_XATTR_H && HAVE_LIBACL */
+	TST_TEST_TCONF("<sys/xattr.h> or <sys/acl.h> does not exist.");
+#endif

-- 
2.50.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22  6:55 ` [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility Andrea Cervesato
@ 2025-07-22 10:51   ` Petr Vorel
  2025-07-22 12:06   ` Petr Vorel
  2025-07-22 19:12   ` Wei Gao via ltp
  2 siblings, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2025-07-22 10:51 UTC (permalink / raw)
  To: Andrea Cervesato; +Cc: ltp

Hi Andrea,

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Thanks!

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22  6:55 ` [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility Andrea Cervesato
  2025-07-22 10:51   ` Petr Vorel
@ 2025-07-22 12:06   ` Petr Vorel
  2025-07-22 13:18     ` Stephen Smalley
  2025-07-23 11:19     ` Andrea Cervesato via ltp
  2025-07-22 19:12   ` Wei Gao via ltp
  2 siblings, 2 replies; 20+ messages in thread
From: Petr Vorel @ 2025-07-22 12:06 UTC (permalink / raw)
  To: Andrea Cervesato; +Cc: Stephen Smalley, ltp

Hi Andrea, all,

[ Cc Stephen, the fix author in case I'm wrong with reproducing on enforcing=0 ]

> Add tst_selinux_enabled() utility in tst_security.h in order to verify
> if SELinux is currently up and running in the system.
...
> +int tst_selinux_enabled(void)
> +{
> +	int res = 0;
> +
> +	if (tst_is_mounted(SELINUX_PATH))
> +		res = 1;

I was wondering if it the test require enforcing or not therefore I retested it
and it's really reproducible with permissive mode, i.e. with kernel command line
security=selinux selinux=1 enforcing=0

Because if enforcing was required, I would be for using tst_selinux_enforcing(),
which checks /sys/fs/selinux/enforce for 1 as Wei suggested in v3:

https://lore.kernel.org/ltp/aHf839WS0BPIa5Zq@MiWiFi-CR6608-srv/

@Cyril @Andrea, just checking if /sys/fs/selinux/enforce exists would be faster
than looping /proc/mounts (via tst_is_mounted(SELINUX_PATH)). Can we just modify
the patch?

Kind regards,
Petr

+++ lib/tst_security.c
@@ -107,7 +107,7 @@ int tst_selinux_enabled(void)
 {
 	int res = 0;
 
-	if (tst_is_mounted(SELINUX_PATH))
+	if (access(SELINUX_STATUS_PATH, F_OK) == 0)
 		res = 1;
 
 	tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22 12:06   ` Petr Vorel
@ 2025-07-22 13:18     ` Stephen Smalley
  2025-07-22 13:20       ` Stephen Smalley
  2025-07-23 11:19     ` Andrea Cervesato via ltp
  1 sibling, 1 reply; 20+ messages in thread
From: Stephen Smalley @ 2025-07-22 13:18 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp

On Tue, Jul 22, 2025 at 8:06 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Andrea, all,
>
> [ Cc Stephen, the fix author in case I'm wrong with reproducing on enforcing=0 ]
>
> > Add tst_selinux_enabled() utility in tst_security.h in order to verify
> > if SELinux is currently up and running in the system.
> ...
> > +int tst_selinux_enabled(void)
> > +{
> > +     int res = 0;
> > +
> > +     if (tst_is_mounted(SELINUX_PATH))
> > +             res = 1;
>
> I was wondering if it the test require enforcing or not therefore I retested it
> and it's really reproducible with permissive mode, i.e. with kernel command line
> security=selinux selinux=1 enforcing=0
>
> Because if enforcing was required, I would be for using tst_selinux_enforcing(),
> which checks /sys/fs/selinux/enforce for 1 as Wei suggested in v3:
>
> https://lore.kernel.org/ltp/aHf839WS0BPIa5Zq@MiWiFi-CR6608-srv/
>
> @Cyril @Andrea, just checking if /sys/fs/selinux/enforce exists would be faster
> than looping /proc/mounts (via tst_is_mounted(SELINUX_PATH)). Can we just modify
> the patch?

Not sure if I have the full context, but modern libselinux has this
for is_selinux_enabled():

int is_selinux_enabled(void)
{
        /* init_selinuxmnt() gets called before this function. We
         * will assume that if a selinux file system is mounted, then
         * selinux is enabled. */
#ifdef ANDROID
        return (selinux_mnt ? 1 : 0);
#else
        return (selinux_mnt && has_selinux_config);
#endif
}

And init_selinuxmnt(), a constructor for libselinux, does this:

static void init_selinuxmnt(void)
{
        char *buf = NULL, *p;
        FILE *fp = NULL;
        size_t len;
        ssize_t num;

        if (selinux_mnt)
                return;

        if (verify_selinuxmnt(SELINUXMNT) == 0) return;

        if (verify_selinuxmnt(OLDSELINUXMNT) == 0) return;

        /* Drop back to detecting it the long way. */
        if (!selinuxfs_exists())
                goto out;
...

So in the common case we don't need to check /proc/mounts or
/proc/filesystems, only if we don't find selinuxfs on one of the
expected mount directories (/sys/fs/selinux or /selinux).

>
> Kind regards,
> Petr
>
> +++ lib/tst_security.c
> @@ -107,7 +107,7 @@ int tst_selinux_enabled(void)
>  {
>         int res = 0;
>
> -       if (tst_is_mounted(SELINUX_PATH))
> +       if (access(SELINUX_STATUS_PATH, F_OK) == 0)
>                 res = 1;
>
>         tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22 13:18     ` Stephen Smalley
@ 2025-07-22 13:20       ` Stephen Smalley
  2025-07-22 13:42         ` Petr Vorel
  0 siblings, 1 reply; 20+ messages in thread
From: Stephen Smalley @ 2025-07-22 13:20 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp

On Tue, Jul 22, 2025 at 9:18 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Jul 22, 2025 at 8:06 AM Petr Vorel <pvorel@suse.cz> wrote:
> >
> > Hi Andrea, all,
> >
> > [ Cc Stephen, the fix author in case I'm wrong with reproducing on enforcing=0 ]
> >
> > > Add tst_selinux_enabled() utility in tst_security.h in order to verify
> > > if SELinux is currently up and running in the system.
> > ...
> > > +int tst_selinux_enabled(void)
> > > +{
> > > +     int res = 0;
> > > +
> > > +     if (tst_is_mounted(SELINUX_PATH))
> > > +             res = 1;
> >
> > I was wondering if it the test require enforcing or not therefore I retested it
> > and it's really reproducible with permissive mode, i.e. with kernel command line
> > security=selinux selinux=1 enforcing=0
> >
> > Because if enforcing was required, I would be for using tst_selinux_enforcing(),
> > which checks /sys/fs/selinux/enforce for 1 as Wei suggested in v3:
> >
> > https://lore.kernel.org/ltp/aHf839WS0BPIa5Zq@MiWiFi-CR6608-srv/
> >
> > @Cyril @Andrea, just checking if /sys/fs/selinux/enforce exists would be faster
> > than looping /proc/mounts (via tst_is_mounted(SELINUX_PATH)). Can we just modify
> > the patch?
>
> Not sure if I have the full context, but modern libselinux has this
> for is_selinux_enabled():
>
> int is_selinux_enabled(void)
> {
>         /* init_selinuxmnt() gets called before this function. We
>          * will assume that if a selinux file system is mounted, then
>          * selinux is enabled. */
> #ifdef ANDROID
>         return (selinux_mnt ? 1 : 0);
> #else
>         return (selinux_mnt && has_selinux_config);
> #endif
> }
>
> And init_selinuxmnt(), a constructor for libselinux, does this:
>
> static void init_selinuxmnt(void)
> {
>         char *buf = NULL, *p;
>         FILE *fp = NULL;
>         size_t len;
>         ssize_t num;
>
>         if (selinux_mnt)
>                 return;
>
>         if (verify_selinuxmnt(SELINUXMNT) == 0) return;
>
>         if (verify_selinuxmnt(OLDSELINUXMNT) == 0) return;
>
>         /* Drop back to detecting it the long way. */
>         if (!selinuxfs_exists())
>                 goto out;
> ...
>
> So in the common case we don't need to check /proc/mounts or
> /proc/filesystems, only if we don't find selinuxfs on one of the
> expected mount directories (/sys/fs/selinux or /selinux).

Also, for reference, verify_selinuxmnt() is as follows:

static int verify_selinuxmnt(const char *mnt)
{
        struct statfs sfbuf;
        int rc;

        do {
                rc = statfs(mnt, &sfbuf);
        } while (rc < 0 && errno == EINTR);
        if (rc == 0) {
                if ((uint32_t)sfbuf.f_type == (uint32_t)SELINUX_MAGIC) {
                        struct statvfs vfsbuf;
                        rc = statvfs(mnt, &vfsbuf);
                        if (rc == 0) {
                                if (!(vfsbuf.f_flag & ST_RDONLY)) {
                                        set_selinuxmnt(mnt);
                                }
                                return 0;
                        }
                }
        }

        return -1;
}

>
> >
> > Kind regards,
> > Petr
> >
> > +++ lib/tst_security.c
> > @@ -107,7 +107,7 @@ int tst_selinux_enabled(void)
> >  {
> >         int res = 0;
> >
> > -       if (tst_is_mounted(SELINUX_PATH))
> > +       if (access(SELINUX_STATUS_PATH, F_OK) == 0)
> >                 res = 1;
> >
> >         tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22 13:20       ` Stephen Smalley
@ 2025-07-22 13:42         ` Petr Vorel
  2025-07-22 13:48           ` Stephen Smalley
  0 siblings, 1 reply; 20+ messages in thread
From: Petr Vorel @ 2025-07-22 13:42 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: ltp

Hi Stephen, all,

> On Tue, Jul 22, 2025 at 9:18 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:

> > On Tue, Jul 22, 2025 at 8:06 AM Petr Vorel <pvorel@suse.cz> wrote:

> > > Hi Andrea, all,

> > > [ Cc Stephen, the fix author in case I'm wrong with reproducing on enforcing=0 ]

> > > > Add tst_selinux_enabled() utility in tst_security.h in order to verify
> > > > if SELinux is currently up and running in the system.
> > > ...
> > > > +int tst_selinux_enabled(void)
> > > > +{
> > > > +     int res = 0;
> > > > +
> > > > +     if (tst_is_mounted(SELINUX_PATH))
> > > > +             res = 1;

> > > I was wondering if it the test require enforcing or not therefore I retested it
> > > and it's really reproducible with permissive mode, i.e. with kernel command line
> > > security=selinux selinux=1 enforcing=0

> > > Because if enforcing was required, I would be for using tst_selinux_enforcing(),
> > > which checks /sys/fs/selinux/enforce for 1 as Wei suggested in v3:

> > > https://lore.kernel.org/ltp/aHf839WS0BPIa5Zq@MiWiFi-CR6608-srv/

> > > @Cyril @Andrea, just checking if /sys/fs/selinux/enforce exists would be faster
> > > than looping /proc/mounts (via tst_is_mounted(SELINUX_PATH)). Can we just modify
> > > the patch?

> > Not sure if I have the full context, but modern libselinux has this
> > for is_selinux_enabled():

> > int is_selinux_enabled(void)
> > {
> >         /* init_selinuxmnt() gets called before this function. We
> >          * will assume that if a selinux file system is mounted, then
> >          * selinux is enabled. */
> > #ifdef ANDROID
> >         return (selinux_mnt ? 1 : 0);
> > #else
> >         return (selinux_mnt && has_selinux_config);
> > #endif
> > }

Thanks for your points.  Yes, we also looked into selinux sources how
selinux_mnt is assigned.

FYI (probably obvious) we don't want to use libselinux to keep LTP dependencies
minimal. Also, we don't really need that precise checks as detecting properly in
chroot etc which is in the libselinux (in selinux_init_load_policy()). IMHO my
suggestion for checking if /sys/fs/selinux/enforce exists (for enabled SELinux
regardless if enforcing or permissive) or whether the value is 1 (for enabled
SELinux enforcement) is enough.

> > And init_selinuxmnt(), a constructor for libselinux, does this:

> > static void init_selinuxmnt(void)
> > {
> >         char *buf = NULL, *p;
> >         FILE *fp = NULL;
> >         size_t len;
> >         ssize_t num;

> >         if (selinux_mnt)
> >                 return;

> >         if (verify_selinuxmnt(SELINUXMNT) == 0) return;

> >         if (verify_selinuxmnt(OLDSELINUXMNT) == 0) return;

> >         /* Drop back to detecting it the long way. */
> >         if (!selinuxfs_exists())
> >                 goto out;
> > ...

> > So in the common case we don't need to check /proc/mounts or
> > /proc/filesystems, only if we don't find selinuxfs on one of the
> > expected mount directories (/sys/fs/selinux or /selinux).

> Also, for reference, verify_selinuxmnt() is as follows:

> static int verify_selinuxmnt(const char *mnt)
> {
>         struct statfs sfbuf;
>         int rc;

>         do {
>                 rc = statfs(mnt, &sfbuf);
>         } while (rc < 0 && errno == EINTR);
>         if (rc == 0) {
>                 if ((uint32_t)sfbuf.f_type == (uint32_t)SELINUX_MAGIC) {
>                         struct statvfs vfsbuf;
>                         rc = statvfs(mnt, &vfsbuf);
>                         if (rc == 0) {
>                                 if (!(vfsbuf.f_flag & ST_RDONLY)) {
>                                         set_selinuxmnt(mnt);
>                                 }
>                                 return 0;
>                         }
>                 }
>         }

>         return -1;
> }

Interesting, I was not aware of SELINUX_MAGIC.

For me was more informative how the setup works to see selinux_init_load_policy()

https://github.com/SELinuxProject/selinux/tree/main/libselinux/src/load_policy.c

Kind regards,
Petr
...

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22 13:42         ` Petr Vorel
@ 2025-07-22 13:48           ` Stephen Smalley
  2025-07-23 11:17             ` Andrea Cervesato via ltp
  0 siblings, 1 reply; 20+ messages in thread
From: Stephen Smalley @ 2025-07-22 13:48 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp

On Tue, Jul 22, 2025 at 9:42 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Stephen, all,
>
> > On Tue, Jul 22, 2025 at 9:18 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
>
> > > On Tue, Jul 22, 2025 at 8:06 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> > > > Hi Andrea, all,
>
> > > > [ Cc Stephen, the fix author in case I'm wrong with reproducing on enforcing=0 ]
>
> > > > > Add tst_selinux_enabled() utility in tst_security.h in order to verify
> > > > > if SELinux is currently up and running in the system.
> > > > ...
> > > > > +int tst_selinux_enabled(void)
> > > > > +{
> > > > > +     int res = 0;
> > > > > +
> > > > > +     if (tst_is_mounted(SELINUX_PATH))
> > > > > +             res = 1;
>
> > > > I was wondering if it the test require enforcing or not therefore I retested it
> > > > and it's really reproducible with permissive mode, i.e. with kernel command line
> > > > security=selinux selinux=1 enforcing=0
>
> > > > Because if enforcing was required, I would be for using tst_selinux_enforcing(),
> > > > which checks /sys/fs/selinux/enforce for 1 as Wei suggested in v3:
>
> > > > https://lore.kernel.org/ltp/aHf839WS0BPIa5Zq@MiWiFi-CR6608-srv/
>
> > > > @Cyril @Andrea, just checking if /sys/fs/selinux/enforce exists would be faster
> > > > than looping /proc/mounts (via tst_is_mounted(SELINUX_PATH)). Can we just modify
> > > > the patch?
>
> > > Not sure if I have the full context, but modern libselinux has this
> > > for is_selinux_enabled():
>
> > > int is_selinux_enabled(void)
> > > {
> > >         /* init_selinuxmnt() gets called before this function. We
> > >          * will assume that if a selinux file system is mounted, then
> > >          * selinux is enabled. */
> > > #ifdef ANDROID
> > >         return (selinux_mnt ? 1 : 0);
> > > #else
> > >         return (selinux_mnt && has_selinux_config);
> > > #endif
> > > }
>
> Thanks for your points.  Yes, we also looked into selinux sources how
> selinux_mnt is assigned.
>
> FYI (probably obvious) we don't want to use libselinux to keep LTP dependencies
> minimal. Also, we don't really need that precise checks as detecting properly in
> chroot etc which is in the libselinux (in selinux_init_load_policy()). IMHO my
> suggestion for checking if /sys/fs/selinux/enforce exists (for enabled SELinux
> regardless if enforcing or permissive) or whether the value is 1 (for enabled
> SELinux enforcement) is enough.

I guess my only question here is whether the tests all require SELinux
to be enforcing or if some of them are just exercising SELinux
functionality that would also pass when permissive. And whether you
care about testing that case.

>
> > > And init_selinuxmnt(), a constructor for libselinux, does this:
>
> > > static void init_selinuxmnt(void)
> > > {
> > >         char *buf = NULL, *p;
> > >         FILE *fp = NULL;
> > >         size_t len;
> > >         ssize_t num;
>
> > >         if (selinux_mnt)
> > >                 return;
>
> > >         if (verify_selinuxmnt(SELINUXMNT) == 0) return;
>
> > >         if (verify_selinuxmnt(OLDSELINUXMNT) == 0) return;
>
> > >         /* Drop back to detecting it the long way. */
> > >         if (!selinuxfs_exists())
> > >                 goto out;
> > > ...
>
> > > So in the common case we don't need to check /proc/mounts or
> > > /proc/filesystems, only if we don't find selinuxfs on one of the
> > > expected mount directories (/sys/fs/selinux or /selinux).
>
> > Also, for reference, verify_selinuxmnt() is as follows:
>
> > static int verify_selinuxmnt(const char *mnt)
> > {
> >         struct statfs sfbuf;
> >         int rc;
>
> >         do {
> >                 rc = statfs(mnt, &sfbuf);
> >         } while (rc < 0 && errno == EINTR);
> >         if (rc == 0) {
> >                 if ((uint32_t)sfbuf.f_type == (uint32_t)SELINUX_MAGIC) {
> >                         struct statvfs vfsbuf;
> >                         rc = statvfs(mnt, &vfsbuf);
> >                         if (rc == 0) {
> >                                 if (!(vfsbuf.f_flag & ST_RDONLY)) {
> >                                         set_selinuxmnt(mnt);
> >                                 }
> >                                 return 0;
> >                         }
> >                 }
> >         }
>
> >         return -1;
> > }
>
> Interesting, I was not aware of SELINUX_MAGIC.
>
> For me was more informative how the setup works to see selinux_init_load_policy()
>
> https://github.com/SELinuxProject/selinux/tree/main/libselinux/src/load_policy.c
>
> Kind regards,
> Petr
> ...

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 2/2] Add listxattr04 reproducer
  2025-07-22  6:55 ` [LTP] [PATCH v4 2/2] Add listxattr04 reproducer Andrea Cervesato
@ 2025-07-22 14:11   ` Petr Vorel
  0 siblings, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2025-07-22 14:11 UTC (permalink / raw)
  To: Andrea Cervesato
  Cc: linux-fsdevel, selinux, Stephen Smalley, Paul Eggert, ltp

Hi Andrea,

FYI Andrea's LTP reproducer for a bug introduced in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8b0ba61df5a1
and fixed in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=800d0b9b6a8b

> From: Andrea Cervesato <andrea.cervesato@suse.com>

> Test reproducer for a bug introduced in 8b0ba61df5a1 ("fs/xattr.c: fix
> simple_xattr_list to always include security.* xattrs").

> Bug can be reproduced when SELinux and ACL are activated on inodes as
> following:

>     $ touch testfile
>     $ setfacl -m u:myuser:rwx testfile
>     $ getfattr -dm. /tmp/testfile
>     Segmentation fault (core dumped)

> The reason why this happens is that simple_xattr_list() always includes
> security.* xattrs without resetting error flag after
> security_inode_listsecurity(). This results into an incorrect length of the
> returned xattr name if POSIX ACL is also applied on the inode.

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  testcases/kernel/syscalls/listxattr/.gitignore    |   1 +
>  testcases/kernel/syscalls/listxattr/Makefile      |   2 +
>  testcases/kernel/syscalls/listxattr/listxattr04.c | 108 ++++++++++++++++++++++
>  3 files changed, 111 insertions(+)

> diff --git a/testcases/kernel/syscalls/listxattr/.gitignore b/testcases/kernel/syscalls/listxattr/.gitignore
> index be0675a6df0080d176d53d70194442bbc9ed376c..0d672b6ea5eec03aab37ee89316c56e24356c1d9 100644
> --- a/testcases/kernel/syscalls/listxattr/.gitignore
> +++ b/testcases/kernel/syscalls/listxattr/.gitignore
> @@ -1,3 +1,4 @@
>  /listxattr01
>  /listxattr02
>  /listxattr03
> +/listxattr04
> diff --git a/testcases/kernel/syscalls/listxattr/Makefile b/testcases/kernel/syscalls/listxattr/Makefile
> index c2f84b1590fc24a7a98f890ea7706771d944aa79..e96bb3fa4c2c6b14b8d2bc8fd4c475e4789d72fe 100644
> --- a/testcases/kernel/syscalls/listxattr/Makefile
> +++ b/testcases/kernel/syscalls/listxattr/Makefile
> @@ -6,4 +6,6 @@ top_srcdir		?= ../../../..

>  include $(top_srcdir)/include/mk/testcases.mk

> +listxattr04: LDLIBS	+= $(ACL_LIBS)
> +
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/listxattr/listxattr04.c b/testcases/kernel/syscalls/listxattr/listxattr04.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..473ed45b5c2da8ff8e49c513eeb82158ec2dc066
> --- /dev/null
> +++ b/testcases/kernel/syscalls/listxattr/listxattr04.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2025 Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +/*\
> + * Test reproducer for a bug introduced in 8b0ba61df5a1 ("fs/xattr.c: fix
> + * simple_xattr_list to always include security.* xattrs").
> + *
> + * Bug can be reproduced when SELinux and ACL are activated on inodes as
> + * following:
> + *
> + *     $ touch testfile
> + *     $ setfacl -m u:myuser:rwx testfile
> + *     $ getfattr -dm. /tmp/testfile
> + *     Segmentation fault (core dumped)
> + *
> + * The reason why this happens is that simple_xattr_list() always includes
> + * security.* xattrs without resetting error flag after
> + * security_inode_listsecurity(). This results into an incorrect length of the
> + * returned xattr name if POSIX ACL is also applied on the inode.
> + */
> +
> +#include "config.h"
> +#include "tst_test.h"
> +
> +#if defined(HAVE_SYS_XATTR_H) && defined(HAVE_LIBACL)
> +
> +#include <pwd.h>
> +#include <sys/acl.h>
> +#include <sys/xattr.h>
> +
> +#define ACL_PERM        "u::rw-,u:root:rwx,g::r--,o::r--,m::rwx"
> +#define TEST_FILE       "test.bin"
> +
> +static acl_t acl;
> +
> +static void verify_xattr(const int size)
> +{
> +	char buf[size];
> +
> +	memset(buf, 0, sizeof(buf));
> +
> +	if (listxattr(TEST_FILE, buf, size) == -1) {
> +		if (errno != ERANGE)
> +			tst_brk(TBROK | TERRNO, "listxattr() error");

The original verifier from RH bugreport check sizes and also works if size > -1
is returned, but I guess it's not necessary, because Andrea's reproducer works
as expected (fails on affected 6.16-rc1 based openSUSE kernel, works on 6.15.x).

LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Tested-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2369561
> +
> +		tst_res(TFAIL, "listxattr() failed to read attributes length: ERANGE");
> +		return;
> +	}
> +
> +	tst_res(TPASS, "listxattr() correctly read attributes length");
> +}
> +
> +static void run(void)
> +{
> +	int size;
> +
> +	size = listxattr(TEST_FILE, NULL, 0);
> +	if (size == -1)
> +		tst_brk(TBROK | TERRNO, "listxattr() error");
> +
> +	verify_xattr(size);
> +}
> +
> +static void setup(void)
> +{
> +	int res;
> +
> +	if (!tst_selinux_enabled())
> +		tst_brk(TCONF, "SELinux is not enabled");
> +
> +	SAFE_TOUCH(TEST_FILE, 0644, NULL);
> +
> +	acl = acl_from_text(ACL_PERM);
> +	if (!acl)
> +		tst_brk(TBROK | TERRNO, "acl_from_text() failed");
> +
> +	res = acl_set_file(TEST_FILE, ACL_TYPE_ACCESS, acl);
> +	if (res == -1) {
> +		if (errno == EOPNOTSUPP)
> +			tst_brk(TCONF | TERRNO, "acl_set_file()");
> +
> +		tst_brk(TBROK | TERRNO, "acl_set_file(%s) failed", TEST_FILE);
> +	}
> +}
> +
> +static void cleanup(void)
> +{
> +	if (acl)
> +		acl_free(acl);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "800d0b9b6a8b"},
> +		{}
> +	}
> +};
> +
> +#else /* HAVE_SYS_XATTR_H && HAVE_LIBACL */
> +	TST_TEST_TCONF("<sys/xattr.h> or <sys/acl.h> does not exist.");
> +#endif

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22  6:55 ` [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility Andrea Cervesato
  2025-07-22 10:51   ` Petr Vorel
  2025-07-22 12:06   ` Petr Vorel
@ 2025-07-22 19:12   ` Wei Gao via ltp
  2 siblings, 0 replies; 20+ messages in thread
From: Wei Gao via ltp @ 2025-07-22 19:12 UTC (permalink / raw)
  To: Andrea Cervesato; +Cc: ltp

On Tue, Jul 22, 2025 at 08:55:56AM +0200, Andrea Cervesato wrote:
> From: Andrea Cervesato <andrea.cervesato@suse.com>
> 
> Add tst_selinux_enabled() utility in tst_security.h in order to verify
> if SELinux is currently up and running in the system.
> 
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  include/tst_security.h |  1 +
>  lib/tst_security.c     | 15 ++++++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/include/tst_security.h b/include/tst_security.h
> index 5d91f8a98f104b0cafaaf2046bc0ceec06870606..cb5490a896f027245064abebb9d7c36270fd2e8a 100644
> --- a/include/tst_security.h
> +++ b/include/tst_security.h
> @@ -14,5 +14,6 @@ int tst_fips_enabled(void);
>  int tst_lockdown_enabled(void);
>  int tst_secureboot_enabled(void);
>  int tst_selinux_enforcing(void);
> +int tst_selinux_enabled(void);
>  
>  #endif /* TST_SECURITY_H__ */
> diff --git a/lib/tst_security.c b/lib/tst_security.c
> index 7d929fafe729058f55b921bf5cf7806b253496e0..f4669c60fbcafeddcab23835ee8c568a4aab46c3 100644
> --- a/lib/tst_security.c
> +++ b/lib/tst_security.c
> @@ -7,7 +7,8 @@
>  
>  #define PATH_FIPS	"/proc/sys/crypto/fips_enabled"
>  #define PATH_LOCKDOWN	"/sys/kernel/security/lockdown"
> -#define SELINUX_STATUS_PATH "/sys/fs/selinux/enforce"
> +#define SELINUX_PATH "/sys/fs/selinux"
> +#define SELINUX_STATUS_PATH (SELINUX_PATH "/enforce")
>  
>  #if defined(__powerpc64__) || defined(__ppc64__)
>  # define SECUREBOOT_VAR "/proc/device-tree/ibm,secure-boot"
> @@ -102,6 +103,18 @@ int tst_secureboot_enabled(void)
>  	return data[VAR_DATA_SIZE - 1];
>  }
>  
> +int tst_selinux_enabled(void)
> +{
> +	int res = 0;
> +
> +	if (tst_is_mounted(SELINUX_PATH))
> +		res = 1;
> +
> +	tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");
> +
> +	return res;
> +}
> +
>  int tst_selinux_enforcing(void)
>  {
>  	int res = 0;
> 
Reviewed-by: Wei Gao <wegao@suse.com>
> -- 
> 2.50.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22 13:48           ` Stephen Smalley
@ 2025-07-23 11:17             ` Andrea Cervesato via ltp
  2025-07-23 12:51               ` Petr Vorel
  0 siblings, 1 reply; 20+ messages in thread
From: Andrea Cervesato via ltp @ 2025-07-23 11:17 UTC (permalink / raw)
  To: Stephen Smalley, Petr Vorel; +Cc: ltp

On 7/22/25 3:48 PM, Stephen Smalley wrote:
> I guess my only question here is whether the tests all require SELinux
> to be enforcing or if some of them are just exercising SELinux
> functionality that would also pass when permissive. And whether you
> care about testing that case.

Hi Stephen,

this particular reproducer doesn't need more than knowing a LSM is up 
and running, so we can trigger the bug on listxattr.
In our case, we chosen SELinux because it's popular and this approach is 
more practical than checking every single LSM implementation in the 
system, verifying if it's enabled or not, no matter SELinux is 
configured in permissive or enforcing mode.

- Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-22 12:06   ` Petr Vorel
  2025-07-22 13:18     ` Stephen Smalley
@ 2025-07-23 11:19     ` Andrea Cervesato via ltp
  2025-07-23 12:41       ` Petr Vorel
  1 sibling, 1 reply; 20+ messages in thread
From: Andrea Cervesato via ltp @ 2025-07-23 11:19 UTC (permalink / raw)
  To: Petr Vorel, Andrea Cervesato; +Cc: Stephen Smalley, ltp

On 7/22/25 2:06 PM, Petr Vorel wrote:
> +++ lib/tst_security.c
> @@ -107,7 +107,7 @@ int tst_selinux_enabled(void)
>   {
>   	int res = 0;
>   
> -	if (tst_is_mounted(SELINUX_PATH))
> +	if (access(SELINUX_STATUS_PATH, F_OK) == 0)
>   		res = 1;
>   
>   	tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");

This is more or less what I was doing at the beginning, but Cyril 
suggested this approach which is more similar to libselinux. Please, 
check v3.

- Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-23 11:19     ` Andrea Cervesato via ltp
@ 2025-07-23 12:41       ` Petr Vorel
  2025-07-23 12:43         ` Andrea Cervesato via ltp
  0 siblings, 1 reply; 20+ messages in thread
From: Petr Vorel @ 2025-07-23 12:41 UTC (permalink / raw)
  To: Andrea Cervesato; +Cc: Stephen Smalley, ltp

Hi all,

> On 7/22/25 2:06 PM, Petr Vorel wrote:
> > +++ lib/tst_security.c
> > @@ -107,7 +107,7 @@ int tst_selinux_enabled(void)
> >   {
> >   	int res = 0;
> > -	if (tst_is_mounted(SELINUX_PATH))
> > +	if (access(SELINUX_STATUS_PATH, F_OK) == 0)
> >   		res = 1;
> >   	tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");

> This is more or less what I was doing at the beginning, but Cyril suggested
> this approach which is more similar to libselinux. Please, check v3.

Sure :). FYI I did check v3 before replying to v4 (I always try to get to the
context looking into older versions, I also noted v3 in one of my replies :)).

And yes, I think you were right, that's why I raised my concern again.

But ok, to quote it here Cyril's reply in v3:
https://lore.kernel.org/ltp/aG5v6enhdqFGgiBj@yuki.lan/

	> +	if (access(SELINUX_PATH, F_OK) == 0 && !tst_dir_is_empty(SELINUX_PATH, 0))
	> +		res = 1;

	Maybe we we can do tst_is_mounted(SELINUX_PATH) here instead. At least
	that seems to be what is_selinux_enabled() seems to be doing.
---

Enabled SELinux means both /sys/fs/selinux/enforce and mounted /sys/fs/selinux/.
I even checked to boot kernel with SELinux compiled in but disabled in the
command line via 'security=selinux selinux=0 enforcing=0' and the result is
expected: no /sys/fs/selinux directory.

Both ways of checking are OK, just "access(SELINUX_STATUS_PATH, F_OK) == 0" is
the quickest way (given test requires just SELinux enabled, not enforcing mode).

This can be changed before merge. Or, feel free to keep the original version as
it also works.

Kind regards,
Petr

> - Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-23 12:41       ` Petr Vorel
@ 2025-07-23 12:43         ` Andrea Cervesato via ltp
  2025-07-23 12:46           ` Stephen Smalley
  0 siblings, 1 reply; 20+ messages in thread
From: Andrea Cervesato via ltp @ 2025-07-23 12:43 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Stephen Smalley, ltp

On 7/23/25 2:41 PM, Petr Vorel wrote:
> Hi all,
>
>> On 7/22/25 2:06 PM, Petr Vorel wrote:
>>> +++ lib/tst_security.c
>>> @@ -107,7 +107,7 @@ int tst_selinux_enabled(void)
>>>    {
>>>    	int res = 0;
>>> -	if (tst_is_mounted(SELINUX_PATH))
>>> +	if (access(SELINUX_STATUS_PATH, F_OK) == 0)
>>>    		res = 1;
>>>    	tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");
>> This is more or less what I was doing at the beginning, but Cyril suggested
>> this approach which is more similar to libselinux. Please, check v3.
> Sure :). FYI I did check v3 before replying to v4 (I always try to get to the
> context looking into older versions, I also noted v3 in one of my replies :)).
>
> And yes, I think you were right, that's why I raised my concern again.
>
> But ok, to quote it here Cyril's reply in v3:
> https://lore.kernel.org/ltp/aG5v6enhdqFGgiBj@yuki.lan/
>
> 	> +	if (access(SELINUX_PATH, F_OK) == 0 && !tst_dir_is_empty(SELINUX_PATH, 0))
> 	> +		res = 1;
>
> 	Maybe we we can do tst_is_mounted(SELINUX_PATH) here instead. At least
> 	that seems to be what is_selinux_enabled() seems to be doing.
> ---
>
> Enabled SELinux means both /sys/fs/selinux/enforce and mounted /sys/fs/selinux/.
> I even checked to boot kernel with SELinux compiled in but disabled in the
> command line via 'security=selinux selinux=0 enforcing=0' and the result is
> expected: no /sys/fs/selinux directory.
>
> Both ways of checking are OK, just "access(SELINUX_STATUS_PATH, F_OK) == 0" is
> the quickest way (given test requires just SELinux enabled, not enforcing mode).
>
> This can be changed before merge. Or, feel free to keep the original version as
> it also works.
>
> Kind regards,
> Petr
>
>> - Andrea

Ok thanks, I was also thinking that maybe we can verify if certain LSMs 
are enabled via /sys/kernel/security/lsm . At the moment we only check 
for selinux, but eventually we can also verify if apparmor is enabled by 
looking at the list in that file. WDYT?

- Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-23 12:43         ` Andrea Cervesato via ltp
@ 2025-07-23 12:46           ` Stephen Smalley
  2025-07-23 13:13             ` Stephen Smalley
  0 siblings, 1 reply; 20+ messages in thread
From: Stephen Smalley @ 2025-07-23 12:46 UTC (permalink / raw)
  To: Andrea Cervesato; +Cc: ltp

On Wed, Jul 23, 2025 at 8:43 AM Andrea Cervesato
<andrea.cervesato@suse.com> wrote:
>
> On 7/23/25 2:41 PM, Petr Vorel wrote:
> > Hi all,
> >
> >> On 7/22/25 2:06 PM, Petr Vorel wrote:
> >>> +++ lib/tst_security.c
> >>> @@ -107,7 +107,7 @@ int tst_selinux_enabled(void)
> >>>    {
> >>>     int res = 0;
> >>> -   if (tst_is_mounted(SELINUX_PATH))
> >>> +   if (access(SELINUX_STATUS_PATH, F_OK) == 0)
> >>>             res = 1;
> >>>     tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");
> >> This is more or less what I was doing at the beginning, but Cyril suggested
> >> this approach which is more similar to libselinux. Please, check v3.
> > Sure :). FYI I did check v3 before replying to v4 (I always try to get to the
> > context looking into older versions, I also noted v3 in one of my replies :)).
> >
> > And yes, I think you were right, that's why I raised my concern again.
> >
> > But ok, to quote it here Cyril's reply in v3:
> > https://lore.kernel.org/ltp/aG5v6enhdqFGgiBj@yuki.lan/
> >
> >       > +     if (access(SELINUX_PATH, F_OK) == 0 && !tst_dir_is_empty(SELINUX_PATH, 0))
> >       > +             res = 1;
> >
> >       Maybe we we can do tst_is_mounted(SELINUX_PATH) here instead. At least
> >       that seems to be what is_selinux_enabled() seems to be doing.
> > ---
> >
> > Enabled SELinux means both /sys/fs/selinux/enforce and mounted /sys/fs/selinux/.
> > I even checked to boot kernel with SELinux compiled in but disabled in the
> > command line via 'security=selinux selinux=0 enforcing=0' and the result is
> > expected: no /sys/fs/selinux directory.
> >
> > Both ways of checking are OK, just "access(SELINUX_STATUS_PATH, F_OK) == 0" is
> > the quickest way (given test requires just SELinux enabled, not enforcing mode).
> >
> > This can be changed before merge. Or, feel free to keep the original version as
> > it also works.
> >
> > Kind regards,
> > Petr
> >
> >> - Andrea
>
> Ok thanks, I was also thinking that maybe we can verify if certain LSMs
> are enabled via /sys/kernel/security/lsm . At the moment we only check
> for selinux, but eventually we can also verify if apparmor is enabled by
> looking at the list in that file. WDYT?

If this is testing for the listxattr bug, then you can only test for
LSMs that implement their own xattr, which would be SELinux and Smack.
AppArmor doesn't implement an xattr, at least not upstream.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-23 11:17             ` Andrea Cervesato via ltp
@ 2025-07-23 12:51               ` Petr Vorel
  0 siblings, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2025-07-23 12:51 UTC (permalink / raw)
  To: Andrea Cervesato; +Cc: Stephen Smalley, ltp

Hi all,

> On 7/22/25 3:48 PM, Stephen Smalley wrote:
> > I guess my only question here is whether the tests all require SELinux
> > to be enforcing or if some of them are just exercising SELinux
> > functionality that would also pass when permissive. And whether you
> > care about testing that case.

> Hi Stephen,

> this particular reproducer doesn't need more than knowing a LSM is up and
> running, so we can trigger the bug on listxattr.
> In our case, we chosen SELinux because it's popular and this approach is
> more practical than checking every single LSM implementation in the system,
> verifying if it's enabled or not, no matter SELinux is configured in
> permissive or enforcing mode.

Thanks for info. Feel free to merge this as is and add support for other LSM
later. Either you send a patch later or please share what's needed to be done.

FYI with disabled SELinux and using other LSM
$ cat /proc/cmdline
... BOOT_IMAGE=/boot/vmlinuz-6.16.0-rc1-236-g8c6bc74c7f891-1.ga27462d-vanilla security=selinux selinux=0 enforcing=0 ima_policy=critical_data

cat /sys/kernel/security/lsm
lockdown,capability,landlock,yama,bpf,ima,evm

and with modified your test to not check for SELinux does not reproduce the bug:
# ./listxattr04
...
listxattr04.c:52: TPASS: listxattr() correctly read attributes length

I suppose at least capability, IMA, EVM and BPF aren't triggering it (there are
definitely used by the kernel). Maybe it requires AppArmor or use other LSM
(which might require their own kernel command line).
Because any LSM means any kernel would be vulnerable. If it's just SELinux and
AppArmor fewer kernels are affected.

Kind regards,
Petr

> - Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-23 12:46           ` Stephen Smalley
@ 2025-07-23 13:13             ` Stephen Smalley
  2025-07-23 13:15               ` Andrea Cervesato via ltp
  0 siblings, 1 reply; 20+ messages in thread
From: Stephen Smalley @ 2025-07-23 13:13 UTC (permalink / raw)
  To: Andrea Cervesato; +Cc: ltp

On Wed, Jul 23, 2025 at 8:46 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, Jul 23, 2025 at 8:43 AM Andrea Cervesato
> <andrea.cervesato@suse.com> wrote:
> >
> > On 7/23/25 2:41 PM, Petr Vorel wrote:
> > > Hi all,
> > >
> > >> On 7/22/25 2:06 PM, Petr Vorel wrote:
> > >>> +++ lib/tst_security.c
> > >>> @@ -107,7 +107,7 @@ int tst_selinux_enabled(void)
> > >>>    {
> > >>>     int res = 0;
> > >>> -   if (tst_is_mounted(SELINUX_PATH))
> > >>> +   if (access(SELINUX_STATUS_PATH, F_OK) == 0)
> > >>>             res = 1;
> > >>>     tst_res(TINFO, "SELinux enabled: %s", res ? "yes" : "no");
> > >> This is more or less what I was doing at the beginning, but Cyril suggested
> > >> this approach which is more similar to libselinux. Please, check v3.
> > > Sure :). FYI I did check v3 before replying to v4 (I always try to get to the
> > > context looking into older versions, I also noted v3 in one of my replies :)).
> > >
> > > And yes, I think you were right, that's why I raised my concern again.
> > >
> > > But ok, to quote it here Cyril's reply in v3:
> > > https://lore.kernel.org/ltp/aG5v6enhdqFGgiBj@yuki.lan/
> > >
> > >       > +     if (access(SELINUX_PATH, F_OK) == 0 && !tst_dir_is_empty(SELINUX_PATH, 0))
> > >       > +             res = 1;
> > >
> > >       Maybe we we can do tst_is_mounted(SELINUX_PATH) here instead. At least
> > >       that seems to be what is_selinux_enabled() seems to be doing.
> > > ---
> > >
> > > Enabled SELinux means both /sys/fs/selinux/enforce and mounted /sys/fs/selinux/.
> > > I even checked to boot kernel with SELinux compiled in but disabled in the
> > > command line via 'security=selinux selinux=0 enforcing=0' and the result is
> > > expected: no /sys/fs/selinux directory.
> > >
> > > Both ways of checking are OK, just "access(SELINUX_STATUS_PATH, F_OK) == 0" is
> > > the quickest way (given test requires just SELinux enabled, not enforcing mode).
> > >
> > > This can be changed before merge. Or, feel free to keep the original version as
> > > it also works.
> > >
> > > Kind regards,
> > > Petr
> > >
> > >> - Andrea
> >
> > Ok thanks, I was also thinking that maybe we can verify if certain LSMs
> > are enabled via /sys/kernel/security/lsm . At the moment we only check
> > for selinux, but eventually we can also verify if apparmor is enabled by
> > looking at the list in that file. WDYT?
>
> If this is testing for the listxattr bug, then you can only test for
> LSMs that implement their own xattr, which would be SELinux and Smack.
> AppArmor doesn't implement an xattr, at least not upstream.

To be more precise, the bug is only triggerable for LSMs that
implement the listsecurity LSM hook (to return a synthesized security
xattr regardless of whether one is set in the filesystem), which are
only SELinux and Smack.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-23 13:13             ` Stephen Smalley
@ 2025-07-23 13:15               ` Andrea Cervesato via ltp
  2025-07-23 19:50                 ` Petr Vorel
  0 siblings, 1 reply; 20+ messages in thread
From: Andrea Cervesato via ltp @ 2025-07-23 13:15 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: ltp


On 7/23/25 3:13 PM, Stephen Smalley wrote:
> To be more precise, the bug is only triggerable for LSMs that
> implement the listsecurity LSM hook (to return a synthesized security
> xattr regardless of whether one is set in the filesystem), which are
> only SELinux and Smack.

Thanks for the clarification. I guess we have a solution: we can take a 
look at /sys/kernel/security/lsm and verify if smack/selinux are enabled.

- Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility
  2025-07-23 13:15               ` Andrea Cervesato via ltp
@ 2025-07-23 19:50                 ` Petr Vorel
  0 siblings, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2025-07-23 19:50 UTC (permalink / raw)
  To: Andrea Cervesato; +Cc: Stephen Smalley, ltp

Hi Andrea, Stephen,

> On 7/23/25 3:13 PM, Stephen Smalley wrote:
> > To be more precise, the bug is only triggerable for LSMs that
> > implement the listsecurity LSM hook (to return a synthesized security
> > xattr regardless of whether one is set in the filesystem), which are
> > only SELinux and Smack.

+1

> Thanks for the clarification. I guess we have a solution: we can take a look
> at /sys/kernel/security/lsm and verify if smack/selinux are enabled.

Given that "selinux" is *not* in /sys/kernel/security/lsm when "security=selinux
selinux=0 enforcing=0" as kernel cmdline and it on "security=selinux selinux=1
enforcing=0" this is really the best solution.

Thank you both for your patience to get a reliable test!

Kind regards,
Petr

> - Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2025-07-23 19:50 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-22  6:55 [LTP] [PATCH v4 0/2] Add listxattr04 test reproducer Andrea Cervesato
2025-07-22  6:55 ` [LTP] [PATCH v4 1/2] core: add tst_selinux_enabled() utility Andrea Cervesato
2025-07-22 10:51   ` Petr Vorel
2025-07-22 12:06   ` Petr Vorel
2025-07-22 13:18     ` Stephen Smalley
2025-07-22 13:20       ` Stephen Smalley
2025-07-22 13:42         ` Petr Vorel
2025-07-22 13:48           ` Stephen Smalley
2025-07-23 11:17             ` Andrea Cervesato via ltp
2025-07-23 12:51               ` Petr Vorel
2025-07-23 11:19     ` Andrea Cervesato via ltp
2025-07-23 12:41       ` Petr Vorel
2025-07-23 12:43         ` Andrea Cervesato via ltp
2025-07-23 12:46           ` Stephen Smalley
2025-07-23 13:13             ` Stephen Smalley
2025-07-23 13:15               ` Andrea Cervesato via ltp
2025-07-23 19:50                 ` Petr Vorel
2025-07-22 19:12   ` Wei Gao via ltp
2025-07-22  6:55 ` [LTP] [PATCH v4 2/2] Add listxattr04 reproducer Andrea Cervesato
2025-07-22 14:11   ` Petr Vorel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox