[PATCH v2 1/8] landlock: Move code to ease future backports

[PATCH v2 1/8] landlock: Move code to ease future backports

[Mickaël Salaün]

To ease backports in setup.c, let's group changes from
__lsm_ro_after_init to __ro_after_init with commit f22f9aaf6c3d
("selinux: remove the runtime disable functionality"), and the
landlock_lsmid addition with commit f3b8788cde61 ("LSM: Identify modules
by more than name").

That will help to backport the following errata.

Cc: Günther Noack <gnoack@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250318161443.279194-2-mic@digikod.net
---

Changes since v1:
- New patch.
---
 security/landlock/setup.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/security/landlock/setup.c b/security/landlock/setup.c
index 28519a45b11f..c71832a8e369 100644
--- a/security/landlock/setup.c
+++ b/security/landlock/setup.c
@@ -19,6 +19,11 @@
 
 bool landlock_initialized __ro_after_init = false;
 
+const struct lsm_id landlock_lsmid = {
+	.name = LANDLOCK_NAME,
+	.id = LSM_ID_LANDLOCK,
+};
+
 struct lsm_blob_sizes landlock_blob_sizes __ro_after_init = {
 	.lbs_cred = sizeof(struct landlock_cred_security),
 	.lbs_file = sizeof(struct landlock_file_security),
@@ -26,11 +31,6 @@ struct lsm_blob_sizes landlock_blob_sizes __ro_after_init = {
 	.lbs_superblock = sizeof(struct landlock_superblock_security),
 };
 
-const struct lsm_id landlock_lsmid = {
-	.name = LANDLOCK_NAME,
-	.id = LSM_ID_LANDLOCK,
-};
-
 static int __init landlock_init(void)
 {
 	landlock_add_cred_hooks();
-- 
2.48.1

[PATCH v2 2/8] landlock: Add the errata interface

[Mickaël Salaün]

Some fixes may require user space to check if they are applied on the
running kernel before using a specific feature.  For instance, this
applies when a restriction was previously too restrictive and is now
getting relaxed (e.g. for compatibility reasons).  However, non-visible
changes for legitimate use (e.g. security fixes) do not require an
erratum.

Because fixes are backported down to a specific Landlock ABI, we need a
way to avoid cherry-pick conflicts.  The solution is to only update a
file related to the lower ABI impacted by this issue.  All the ABI files
are then used to create a bitmask of fixes.

The new errata interface is similar to the one used to get the supported
Landlock ABI version, but it returns a bitmask instead because the order
of fixes may not match the order of versions, and not all fixes may
apply to all versions.

The actual errata will come with dedicated commits.  The description is
not actually used in the code but serves as documentation.

Create the landlock_abi_version symbol and use its value to check errata
consistency.

Update test_base's create_ruleset_checks_ordering tests and add errata
tests.

This commit is backportable down to the first version of Landlock.

Fixes: 3532b0b4352c ("landlock: Enable user space to infer supported features")
Cc: Günther Noack <gnoack@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250318161443.279194-3-mic@digikod.net
---

Changes since v1:
- New patch.
---
 include/uapi/linux/landlock.h                |  2 +
 security/landlock/errata.h                   | 87 ++++++++++++++++++++
 security/landlock/setup.c                    | 30 +++++++
 security/landlock/setup.h                    |  3 +
 security/landlock/syscalls.c                 | 22 ++++-
 tools/testing/selftests/landlock/base_test.c | 38 ++++++++-
 6 files changed, 177 insertions(+), 5 deletions(-)
 create mode 100644 security/landlock/errata.h

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index e1d2c27533b4..8806a132d7b8 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -57,9 +57,11 @@ struct landlock_ruleset_attr {
  *
  * - %LANDLOCK_CREATE_RULESET_VERSION: Get the highest supported Landlock ABI
  *   version.
+ * - %LANDLOCK_CREATE_RULESET_ERRATA: Get a bitmask of fixed issues.
  */
 /* clang-format off */
 #define LANDLOCK_CREATE_RULESET_VERSION			(1U << 0)
+#define LANDLOCK_CREATE_RULESET_ERRATA			(1U << 1)
 /* clang-format on */
 
 /**
diff --git a/security/landlock/errata.h b/security/landlock/errata.h
new file mode 100644
index 000000000000..f26b28b9873d
--- /dev/null
+++ b/security/landlock/errata.h
@@ -0,0 +1,87 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Landlock - Errata information
+ *
+ * Copyright © 2025 Microsoft Corporation
+ */
+
+#ifndef _SECURITY_LANDLOCK_ERRATA_H
+#define _SECURITY_LANDLOCK_ERRATA_H
+
+#include <linux/init.h>
+
+struct landlock_erratum {
+	const int abi;
+	const u8 number;
+};
+
+/* clang-format off */
+#define LANDLOCK_ERRATUM(NUMBER) \
+	{ \
+		.abi = LANDLOCK_ERRATA_ABI, \
+		.number = NUMBER, \
+	},
+/* clang-format on */
+
+/*
+ * Some fixes may require user space to check if they are applied on the running
+ * kernel before using a specific feature.  For instance, this applies when a
+ * restriction was previously too restrictive and is now getting relaxed (for
+ * compatibility or semantic reasons).  However, non-visible changes for
+ * legitimate use (e.g. security fixes) do not require an erratum.
+ */
+static const struct landlock_erratum landlock_errata_init[] __initconst = {
+
+/*
+ * Only Sparse may not implement __has_include.  If a compiler does not
+ * implement __has_include, a warning will be printed at boot time (see
+ * setup.c).
+ */
+#ifdef __has_include
+
+#define LANDLOCK_ERRATA_ABI 1
+#if __has_include("errata/abi-1.h")
+#include "errata/abi-1.h"
+#endif
+#undef LANDLOCK_ERRATA_ABI
+
+#define LANDLOCK_ERRATA_ABI 2
+#if __has_include("errata/abi-2.h")
+#include "errata/abi-2.h"
+#endif
+#undef LANDLOCK_ERRATA_ABI
+
+#define LANDLOCK_ERRATA_ABI 3
+#if __has_include("errata/abi-3.h")
+#include "errata/abi-3.h"
+#endif
+#undef LANDLOCK_ERRATA_ABI
+
+#define LANDLOCK_ERRATA_ABI 4
+#if __has_include("errata/abi-4.h")
+#include "errata/abi-4.h"
+#endif
+#undef LANDLOCK_ERRATA_ABI
+
+/*
+ * For each new erratum, we need to include all the ABI files up to the impacted
+ * ABI to make all potential future intermediate errata easy to backport.
+ *
+ * If such change involves more than one ABI addition, then it must be in a
+ * dedicated commit with the same Fixes tag as used for the actual fix.
+ *
+ * Each commit creating a new security/landlock/errata/abi-*.h file must have a
+ * Depends-on tag to reference the commit that previously added the line to
+ * include this new file, except if the original Fixes tag is enough.
+ *
+ * Each erratum must be documented in its related ABI file, and a dedicated
+ * commit must update Documentation/userspace-api/landlock.rst to include this
+ * erratum.  This commit will not be backported.
+ */
+
+#endif
+
+	{}
+};
+
+#endif /* _SECURITY_LANDLOCK_ERRATA_H */
diff --git a/security/landlock/setup.c b/security/landlock/setup.c
index c71832a8e369..0c85ea27e409 100644
--- a/security/landlock/setup.c
+++ b/security/landlock/setup.c
@@ -6,12 +6,14 @@
  * Copyright © 2018-2020 ANSSI
  */
 
+#include <linux/bits.h>
 #include <linux/init.h>
 #include <linux/lsm_hooks.h>
 #include <uapi/linux/lsm.h>
 
 #include "common.h"
 #include "cred.h"
+#include "errata.h"
 #include "fs.h"
 #include "net.h"
 #include "setup.h"
@@ -31,8 +33,36 @@ struct lsm_blob_sizes landlock_blob_sizes __ro_after_init = {
 	.lbs_superblock = sizeof(struct landlock_superblock_security),
 };
 
+int landlock_errata __ro_after_init;
+
+static void __init compute_errata(void)
+{
+	size_t i;
+
+#ifndef __has_include
+	/*
+	 * This is a safeguard to make sure the compiler implements
+	 * __has_include (see errata.h).
+	 */
+	WARN_ON_ONCE(1);
+	return;
+#endif
+
+	for (i = 0; landlock_errata_init[i].number; i++) {
+		const int prev_errata = landlock_errata;
+
+		if (WARN_ON_ONCE(landlock_errata_init[i].abi >
+				 landlock_abi_version))
+			continue;
+
+		landlock_errata |= BIT(landlock_errata_init[i].number - 1);
+		WARN_ON_ONCE(prev_errata == landlock_errata);
+	}
+}
+
 static int __init landlock_init(void)
 {
+	compute_errata();
 	landlock_add_cred_hooks();
 	landlock_add_task_hooks();
 	landlock_add_fs_hooks();
diff --git a/security/landlock/setup.h b/security/landlock/setup.h
index c4252d46d49d..fca307c35fee 100644
--- a/security/landlock/setup.h
+++ b/security/landlock/setup.h
@@ -11,7 +11,10 @@
 
 #include <linux/lsm_hooks.h>
 
+extern const int landlock_abi_version;
+
 extern bool landlock_initialized;
+extern int landlock_errata;
 
 extern struct lsm_blob_sizes landlock_blob_sizes;
 extern const struct lsm_id landlock_lsmid;
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index a9760d252fc2..cf9e0483e542 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -160,7 +160,9 @@ static const struct file_operations ruleset_fops = {
  *        the new ruleset.
  * @size: Size of the pointed &struct landlock_ruleset_attr (needed for
  *        backward and forward compatibility).
- * @flags: Supported value: %LANDLOCK_CREATE_RULESET_VERSION.
+ * @flags: Supported value:
+ *         - %LANDLOCK_CREATE_RULESET_VERSION
+ *         - %LANDLOCK_CREATE_RULESET_ERRATA
  *
  * This system call enables to create a new Landlock ruleset, and returns the
  * related file descriptor on success.
@@ -169,6 +171,10 @@ static const struct file_operations ruleset_fops = {
  * 0, then the returned value is the highest supported Landlock ABI version
  * (starting at 1).
  *
+ * If @flags is %LANDLOCK_CREATE_RULESET_ERRATA and @attr is NULL and @size is
+ * 0, then the returned value is a bitmask of fixed issues for the current
+ * Landlock ABI version.
+ *
  * Possible returned errors are:
  *
  * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
@@ -192,9 +198,15 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
 		return -EOPNOTSUPP;
 
 	if (flags) {
-		if ((flags == LANDLOCK_CREATE_RULESET_VERSION) && !attr &&
-		    !size)
-			return LANDLOCK_ABI_VERSION;
+		if (attr || size)
+			return -EINVAL;
+
+		if (flags == LANDLOCK_CREATE_RULESET_VERSION)
+			return landlock_abi_version;
+
+		if (flags == LANDLOCK_CREATE_RULESET_ERRATA)
+			return landlock_errata;
+
 		return -EINVAL;
 	}
 
@@ -235,6 +247,8 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
 	return ruleset_fd;
 }
 
+const int landlock_abi_version = LANDLOCK_ABI_VERSION;
+
 /*
  * Returns an owned ruleset from a FD. It is thus needed to call
  * landlock_put_ruleset() on the return value.
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 1bc16fde2e8a..c0abadd0bbbe 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -98,10 +98,46 @@ TEST(abi_version)
 	ASSERT_EQ(EINVAL, errno);
 }
 
+TEST(errata)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
+	};
+	int errata;
+
+	errata = landlock_create_ruleset(NULL, 0,
+					 LANDLOCK_CREATE_RULESET_ERRATA);
+	/* The errata bitmask will not be backported to tests. */
+	ASSERT_LE(0, errata);
+	TH_LOG("errata: 0x%x", errata);
+
+	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
+					      LANDLOCK_CREATE_RULESET_ERRATA));
+	ASSERT_EQ(EINVAL, errno);
+
+	ASSERT_EQ(-1, landlock_create_ruleset(NULL, sizeof(ruleset_attr),
+					      LANDLOCK_CREATE_RULESET_ERRATA));
+	ASSERT_EQ(EINVAL, errno);
+
+	ASSERT_EQ(-1,
+		  landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr),
+					  LANDLOCK_CREATE_RULESET_ERRATA));
+	ASSERT_EQ(EINVAL, errno);
+
+	ASSERT_EQ(-1, landlock_create_ruleset(
+			      NULL, 0,
+			      LANDLOCK_CREATE_RULESET_VERSION |
+				      LANDLOCK_CREATE_RULESET_ERRATA));
+	ASSERT_EQ(-1, landlock_create_ruleset(NULL, 0,
+					      LANDLOCK_CREATE_RULESET_ERRATA |
+						      1 << 31));
+	ASSERT_EQ(EINVAL, errno);
+}
+
 /* Tests ordering of syscall argument checks. */
 TEST(create_ruleset_checks_ordering)
 {
-	const int last_flag = LANDLOCK_CREATE_RULESET_VERSION;
+	const int last_flag = LANDLOCK_CREATE_RULESET_ERRATA;
 	const int invalid_flag = last_flag << 1;
 	int ruleset_fd;
 	const struct landlock_ruleset_attr ruleset_attr = {
-- 
2.48.1

Re: [PATCH v2 2/8] landlock: Add the errata interface

[Günther Noack]

Hello!

Thanks for these patches!  An explicit errata versioning is a good
idea.  With that, I could change the Go-Landlock library to only use
the "V6" level ABI on kernels where the signal erratum is present, so
that the libpsx(3)/nptl(7) hacks continue to work.

The userspace API for this looks good to me as well (except for that I
wonder whether we should not expose errata numbers as constants in the
uapi header)?

Regarding internal implementation, splitting the errata definitions up
into multiple headers feels complicated compared to the naive solution
and it's not entirely clear to me what the purpose is (see below).
(Resolving merge conflicts is normally the job of the downstream
kernel maintainers when doing a cherry pick, and the merge conflicts
don't sound too complicated in this case?)


On Tue, Mar 18, 2025 at 05:14:37PM +0100, Mickaël Salaün wrote:
> Some fixes may require user space to check if they are applied on the
> running kernel before using a specific feature.  For instance, this
> applies when a restriction was previously too restrictive and is now
> getting relaxed (e.g. for compatibility reasons).  However, non-visible
> changes for legitimate use (e.g. security fixes) do not require an
> erratum.
> 
> Because fixes are backported down to a specific Landlock ABI, we need a
> way to avoid cherry-pick conflicts.  The solution is to only update a
> file related to the lower ABI impacted by this issue.  All the ABI files
> are then used to create a bitmask of fixes.

I do not fully understand the underlying purpose here.

In this commit, the errata.h header includes errata/abi-[1234].h.  If
this patch can be backported to the first version of Landlock (as the
commit message says), 4 seems like an arbitrary limit, which is
"including headers from the future", in the kernel that it gets
backported to.

If future errata patches (like the one for signals) need to extend the
list of ABIs in errata.h anyway, doesn't that create the same kinds of
potential merge conflicts which we tried to avoid by splitting up the
errata lists into errata/abi-?.h?


What problem are you addressing with the scheme of splitting up the
errata/abi-?.h files?

 (A) Reduced merge conflicts at backporting time?

 (B) Catching the case where a errata patch gets applied to a too old
     kernel for it to make sense?

 (C) Something else that I did not see?


Is there a cherry picking error scenario which can slip through if we
were to use a simpler scheme for storing errata?  The most naive way
would be:

const int landlock_errata = 0 \
	| LANDLOCK_ERRATA_TCP_SOCKET_IDENTIFICATION \
        | LANDLOCK_ERRATA_CROSS_THREAD_SIGNALING \
        ;

In scenario (A), the merge conflict in the definition of
landlock_errata would at worst be a one liner in that const
definition, which sounds doable?  (Obviously, there can still be merge
conflicts in the feature code, that is unavoidable.)

In scenario (B), it is likely to result in a merge conflict in the
feature code anyway?  (The signal code and networking code must
already be there, in order to apply a small change to them.)


> The new errata interface is similar to the one used to get the supported
> Landlock ABI version, but it returns a bitmask instead because the order
> of fixes may not match the order of versions, and not all fixes may
> apply to all versions.
> 
> The actual errata will come with dedicated commits.  The description is
> not actually used in the code but serves as documentation.
> 
> Create the landlock_abi_version symbol and use its value to check errata
> consistency.
> 
> Update test_base's create_ruleset_checks_ordering tests and add errata
> tests.
> 
> This commit is backportable down to the first version of Landlock.
> 
> Fixes: 3532b0b4352c ("landlock: Enable user space to infer supported features")
> Cc: Günther Noack <gnoack@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20250318161443.279194-3-mic@digikod.net
> ---
> 
> Changes since v1:
> - New patch.
> ---
>  include/uapi/linux/landlock.h                |  2 +
>  security/landlock/errata.h                   | 87 ++++++++++++++++++++
>  security/landlock/setup.c                    | 30 +++++++
>  security/landlock/setup.h                    |  3 +
>  security/landlock/syscalls.c                 | 22 ++++-
>  tools/testing/selftests/landlock/base_test.c | 38 ++++++++-
>  6 files changed, 177 insertions(+), 5 deletions(-)
>  create mode 100644 security/landlock/errata.h
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index e1d2c27533b4..8806a132d7b8 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -57,9 +57,11 @@ struct landlock_ruleset_attr {
>   *
>   * - %LANDLOCK_CREATE_RULESET_VERSION: Get the highest supported Landlock ABI
>   *   version.
> + * - %LANDLOCK_CREATE_RULESET_ERRATA: Get a bitmask of fixed issues.
>   */
>  /* clang-format off */
>  #define LANDLOCK_CREATE_RULESET_VERSION			(1U << 0)
> +#define LANDLOCK_CREATE_RULESET_ERRATA			(1U << 1)
>  /* clang-format on */
>  
>  /**
> diff --git a/security/landlock/errata.h b/security/landlock/errata.h
> new file mode 100644
> index 000000000000..f26b28b9873d
> --- /dev/null
> +++ b/security/landlock/errata.h
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Landlock - Errata information
> + *
> + * Copyright © 2025 Microsoft Corporation
> + */
> +
> +#ifndef _SECURITY_LANDLOCK_ERRATA_H
> +#define _SECURITY_LANDLOCK_ERRATA_H
> +
> +#include <linux/init.h>
> +
> +struct landlock_erratum {
> +	const int abi;
> +	const u8 number;
> +};
> +
> +/* clang-format off */
> +#define LANDLOCK_ERRATUM(NUMBER) \
> +	{ \
> +		.abi = LANDLOCK_ERRATA_ABI, \
> +		.number = NUMBER, \
> +	},
> +/* clang-format on */
> +
> +/*
> + * Some fixes may require user space to check if they are applied on the running
> + * kernel before using a specific feature.  For instance, this applies when a
> + * restriction was previously too restrictive and is now getting relaxed (for
> + * compatibility or semantic reasons).  However, non-visible changes for
> + * legitimate use (e.g. security fixes) do not require an erratum.
> + */
> +static const struct landlock_erratum landlock_errata_init[] __initconst = {
> +
> +/*
> + * Only Sparse may not implement __has_include.  If a compiler does not
> + * implement __has_include, a warning will be printed at boot time (see
> + * setup.c).
> + */
> +#ifdef __has_include
> +
> +#define LANDLOCK_ERRATA_ABI 1
> +#if __has_include("errata/abi-1.h")
> +#include "errata/abi-1.h"
> +#endif
> +#undef LANDLOCK_ERRATA_ABI
> +
> +#define LANDLOCK_ERRATA_ABI 2
> +#if __has_include("errata/abi-2.h")
> +#include "errata/abi-2.h"
> +#endif
> +#undef LANDLOCK_ERRATA_ABI
> +
> +#define LANDLOCK_ERRATA_ABI 3
> +#if __has_include("errata/abi-3.h")
> +#include "errata/abi-3.h"
> +#endif
> +#undef LANDLOCK_ERRATA_ABI
> +
> +#define LANDLOCK_ERRATA_ABI 4
> +#if __has_include("errata/abi-4.h")
> +#include "errata/abi-4.h"
> +#endif
> +#undef LANDLOCK_ERRATA_ABI


> +
> +/*
> + * For each new erratum, we need to include all the ABI files up to the impacted
> + * ABI to make all potential future intermediate errata easy to backport.
> + *
> + * If such change involves more than one ABI addition, then it must be in a
> + * dedicated commit with the same Fixes tag as used for the actual fix.
> + *
> + * Each commit creating a new security/landlock/errata/abi-*.h file must have a
> + * Depends-on tag to reference the commit that previously added the line to
> + * include this new file, except if the original Fixes tag is enough.
> + *
> + * Each erratum must be documented in its related ABI file, and a dedicated
> + * commit must update Documentation/userspace-api/landlock.rst to include this
> + * erratum.  This commit will not be backported.
> + */
> +
> +#endif
> +
> +	{}
> +};
> +
> +#endif /* _SECURITY_LANDLOCK_ERRATA_H */
> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
> index c71832a8e369..0c85ea27e409 100644
> --- a/security/landlock/setup.c
> +++ b/security/landlock/setup.c
> @@ -6,12 +6,14 @@
>   * Copyright © 2018-2020 ANSSI
>   */
>  
> +#include <linux/bits.h>
>  #include <linux/init.h>
>  #include <linux/lsm_hooks.h>
>  #include <uapi/linux/lsm.h>
>  
>  #include "common.h"
>  #include "cred.h"
> +#include "errata.h"
>  #include "fs.h"
>  #include "net.h"
>  #include "setup.h"
> @@ -31,8 +33,36 @@ struct lsm_blob_sizes landlock_blob_sizes __ro_after_init = {
>  	.lbs_superblock = sizeof(struct landlock_superblock_security),
>  };
>  
> +int landlock_errata __ro_after_init;
> +
> +static void __init compute_errata(void)
> +{
> +	size_t i;
> +
> +#ifndef __has_include
> +	/*
> +	 * This is a safeguard to make sure the compiler implements
> +	 * __has_include (see errata.h).
> +	 */
> +	WARN_ON_ONCE(1);
> +	return;
> +#endif
> +
> +	for (i = 0; landlock_errata_init[i].number; i++) {
> +		const int prev_errata = landlock_errata;
> +
> +		if (WARN_ON_ONCE(landlock_errata_init[i].abi >
> +				 landlock_abi_version))
> +			continue;

IIUC, if we hit this condition, someone has tried to backport an
erratum that does not apply here?  Shouldn't this ideally be a compile
time error?  Then downstream kernel maintainers would notice it
earlier when they apply the wrong patch?

Can this scenario really happen?  It feels that this should normally
already be caught in merge conflicts at cherry picking time?


> +
> +		landlock_errata |= BIT(landlock_errata_init[i].number - 1);
> +		WARN_ON_ONCE(prev_errata == landlock_errata);
> +	}
> +}
> +
>  static int __init landlock_init(void)
>  {
> +	compute_errata();
>  	landlock_add_cred_hooks();
>  	landlock_add_task_hooks();
>  	landlock_add_fs_hooks();
> diff --git a/security/landlock/setup.h b/security/landlock/setup.h
> index c4252d46d49d..fca307c35fee 100644
> --- a/security/landlock/setup.h
> +++ b/security/landlock/setup.h
> @@ -11,7 +11,10 @@
>  
>  #include <linux/lsm_hooks.h>
>  
> +extern const int landlock_abi_version;
> +
>  extern bool landlock_initialized;
> +extern int landlock_errata;
>  
>  extern struct lsm_blob_sizes landlock_blob_sizes;
>  extern const struct lsm_id landlock_lsmid;
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index a9760d252fc2..cf9e0483e542 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -160,7 +160,9 @@ static const struct file_operations ruleset_fops = {
>   *        the new ruleset.
>   * @size: Size of the pointed &struct landlock_ruleset_attr (needed for
>   *        backward and forward compatibility).
> - * @flags: Supported value: %LANDLOCK_CREATE_RULESET_VERSION.
> + * @flags: Supported value:
> + *         - %LANDLOCK_CREATE_RULESET_VERSION
> + *         - %LANDLOCK_CREATE_RULESET_ERRATA
>   *
>   * This system call enables to create a new Landlock ruleset, and returns the
>   * related file descriptor on success.
> @@ -169,6 +171,10 @@ static const struct file_operations ruleset_fops = {
>   * 0, then the returned value is the highest supported Landlock ABI version
>   * (starting at 1).
>   *
> + * If @flags is %LANDLOCK_CREATE_RULESET_ERRATA and @attr is NULL and @size is
> + * 0, then the returned value is a bitmask of fixed issues for the current
> + * Landlock ABI version.

We should probably say here where that list is defined.

Should the errata numbers also be constants in the
uapi/linux/landlock.h header?

> + *
>   * Possible returned errors are:
>   *
>   * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
> @@ -192,9 +198,15 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>  		return -EOPNOTSUPP;
>  
>  	if (flags) {
> -		if ((flags == LANDLOCK_CREATE_RULESET_VERSION) && !attr &&
> -		    !size)
> -			return LANDLOCK_ABI_VERSION;
> +		if (attr || size)
> +			return -EINVAL;
> +
> +		if (flags == LANDLOCK_CREATE_RULESET_VERSION)
> +			return landlock_abi_version;
> +
> +		if (flags == LANDLOCK_CREATE_RULESET_ERRATA)
> +			return landlock_errata;
> +
>  		return -EINVAL;
>  	}
>  
> @@ -235,6 +247,8 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>  	return ruleset_fd;
>  }
>  
> +const int landlock_abi_version = LANDLOCK_ABI_VERSION;
> +
>  /*
>   * Returns an owned ruleset from a FD. It is thus needed to call
>   * landlock_put_ruleset() on the return value.
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 1bc16fde2e8a..c0abadd0bbbe 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -98,10 +98,46 @@ TEST(abi_version)
>  	ASSERT_EQ(EINVAL, errno);
>  }
>  
> +TEST(errata)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
> +	};
> +	int errata;
> +
> +	errata = landlock_create_ruleset(NULL, 0,
> +					 LANDLOCK_CREATE_RULESET_ERRATA);
> +	/* The errata bitmask will not be backported to tests. */
> +	ASSERT_LE(0, errata);
> +	TH_LOG("errata: 0x%x", errata);
> +
> +	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> +					      LANDLOCK_CREATE_RULESET_ERRATA));
> +	ASSERT_EQ(EINVAL, errno);
> +
> +	ASSERT_EQ(-1, landlock_create_ruleset(NULL, sizeof(ruleset_attr),
> +					      LANDLOCK_CREATE_RULESET_ERRATA));
> +	ASSERT_EQ(EINVAL, errno);
> +
> +	ASSERT_EQ(-1,
> +		  landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr),
> +					  LANDLOCK_CREATE_RULESET_ERRATA));
> +	ASSERT_EQ(EINVAL, errno);
> +
> +	ASSERT_EQ(-1, landlock_create_ruleset(
> +			      NULL, 0,
> +			      LANDLOCK_CREATE_RULESET_VERSION |
> +				      LANDLOCK_CREATE_RULESET_ERRATA));
> +	ASSERT_EQ(-1, landlock_create_ruleset(NULL, 0,
> +					      LANDLOCK_CREATE_RULESET_ERRATA |
> +						      1 << 31));
> +	ASSERT_EQ(EINVAL, errno);

There are two calls to landlock_create_ruleset() here, but only one
ASSERT_EQ(EINVAL, errno).  I do not understand why the second call to
landlock_create_ruleset() was done here (with the | 1 << 31) -- was
that left here by accident?


> +}
> +
>  /* Tests ordering of syscall argument checks. */
>  TEST(create_ruleset_checks_ordering)
>  {
> -	const int last_flag = LANDLOCK_CREATE_RULESET_VERSION;
> +	const int last_flag = LANDLOCK_CREATE_RULESET_ERRATA;
>  	const int invalid_flag = last_flag << 1;
>  	int ruleset_fd;
>  	const struct landlock_ruleset_attr ruleset_attr = {
> -- 
> 2.48.1
> 

–Günther

Re: [PATCH v2 2/8] landlock: Add the errata interface

[Mickaël Salaün]

On Sun, Mar 30, 2025 at 12:13:08PM +0200, Günther Noack wrote:
> Hello!
> 
> Thanks for these patches!  An explicit errata versioning is a good
> idea.  With that, I could change the Go-Landlock library to only use
> the "V6" level ABI on kernels where the signal erratum is present, so
> that the libpsx(3)/nptl(7) hacks continue to work.
> 
> The userspace API for this looks good to me as well (except for that I
> wonder whether we should not expose errata numbers as constants in the
> uapi header)?

We could do that but backporting them would be a pain.  That would need
to come from a standalone commit, which means the UAPI header could not
be used by the kernel.  I'm not sure this is worth it given that they
are still documented in the user doc, similarly as ABI versions.

> 
> Regarding internal implementation, splitting the errata definitions up
> into multiple headers feels complicated compared to the naive solution
> and it's not entirely clear to me what the purpose is (see below).

As explain in the comment and the commit message, the goal is to avoid
conflict when backporting fixes with errata.  If all errata are defined
in the same file, only backporting some of them would be a nightmare.

> (Resolving merge conflicts is normally the job of the downstream
> kernel maintainers when doing a cherry pick, and the merge conflicts
> don't sound too complicated in this case?)

No, backporting fixes to stable branches (and resolving the related
conflicts) is my job, and I want to make my life easier. :)

This is unrelated to distros backporting Landlock *features*, and it
should not change anything for them.

> 
> 
> On Tue, Mar 18, 2025 at 05:14:37PM +0100, Mickaël Salaün wrote:
> > Some fixes may require user space to check if they are applied on the
> > running kernel before using a specific feature.  For instance, this
> > applies when a restriction was previously too restrictive and is now
> > getting relaxed (e.g. for compatibility reasons).  However, non-visible
> > changes for legitimate use (e.g. security fixes) do not require an
> > erratum.
> > 
> > Because fixes are backported down to a specific Landlock ABI, we need a
> > way to avoid cherry-pick conflicts.  The solution is to only update a
> > file related to the lower ABI impacted by this issue.  All the ABI files
> > are then used to create a bitmask of fixes.
> 
> I do not fully understand the underlying purpose here.
> 
> In this commit, the errata.h header includes errata/abi-[1234].h.  If
> this patch can be backported to the first version of Landlock (as the
> commit message says), 4 seems like an arbitrary limit, which is
> "including headers from the future", in the kernel that it gets
> backported to.

This commit *potentially* includes 4 header files, but they are not
provided by this commit.  This works thanks to the __has_include
directive.  It's not an issue to potentially have these files included,
but without them it would be an issue when backporting errata that
affect an ABI version <= 3 because the newer kernel would already have
the related abi-N.h file included (or it would be another conflict to
have a commit that both add an erratum and fix an issue).

I want to avoid patches that will only be created for backports but not
included in the master branch.

> 
> If future errata patches (like the one for signals) need to extend the
> list of ABIs in errata.h anyway, doesn't that create the same kinds of
> potential merge conflicts which we tried to avoid by splitting up the
> errata lists into errata/abi-?.h?

No because errata.h in only growing and all errata in the same abi-N.h
file should be backported together.  As explain in the comment, e.g. if
we need to add errata for ABI v5, we'll only need to backport the commit
that previously included abi-5.h and abi-6.h (with a Depends-on tag),
and to create a new commit that fixes the issue and adds the erratum in
abi-5.h at the same time.

> 
> 
> What problem are you addressing with the scheme of splitting up the
> errata/abi-?.h files?
> 
>  (A) Reduced merge conflicts at backporting time?

The goal is to avoid (not reduce) merge conflicts.

> 
>  (B) Catching the case where a errata patch gets applied to a too old
>      kernel for it to make sense?

No, this can only caught at runtime.

> 
>  (C) Something else that I did not see?

No :)

> 
> 
> Is there a cherry picking error scenario which can slip through if we
> were to use a simpler scheme for storing errata?  The most naive way
> would be:
> 
> const int landlock_errata = 0 \
> 	| LANDLOCK_ERRATA_TCP_SOCKET_IDENTIFICATION \
>         | LANDLOCK_ERRATA_CROSS_THREAD_SIGNALING \
>         ;

There are two issues with this approach:
1/ This line would be subject to conflict for *every* erratum backport,
   or if it is split into several lines: where (in the kernel code) to
   extend this landlock_errata variable?
2/ This only moves (one part of) the problem to where these
   LANDLOCK_ERRATA_* constants are defined.

> 
> In scenario (A), the merge conflict in the definition of
> landlock_errata would at worst be a one liner in that const
> definition, which sounds doable?  (Obviously, there can still be merge
> conflicts in the feature code, that is unavoidable.)

I can handle merge conflicts but I'd much prefer to avoid any. :)

The goal of this patch is to avoid any merge conflict, including in the
feature code.

Some potential future code fixes could conflict when backporting them,
but that will not impact the errata interface.

> 
> In scenario (B), it is likely to result in a merge conflict in the
> feature code anyway?  (The signal code and networking code must
> already be there, in order to apply a small change to them.)

The code to be fixed must be there for this fix to make sense, which
means that if the related abi-N.h file exists, it should be updated the
same way, and if it doesn't exist, the fix commit should create it,
whatever the kernel version.  In both cases, no conflict.

> 
> 
> > The new errata interface is similar to the one used to get the supported
> > Landlock ABI version, but it returns a bitmask instead because the order
> > of fixes may not match the order of versions, and not all fixes may
> > apply to all versions.
> > 
> > The actual errata will come with dedicated commits.  The description is
> > not actually used in the code but serves as documentation.
> > 
> > Create the landlock_abi_version symbol and use its value to check errata
> > consistency.
> > 
> > Update test_base's create_ruleset_checks_ordering tests and add errata
> > tests.
> > 
> > This commit is backportable down to the first version of Landlock.
> > 
> > Fixes: 3532b0b4352c ("landlock: Enable user space to infer supported features")
> > Cc: Günther Noack <gnoack@google.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20250318161443.279194-3-mic@digikod.net
> > ---
> > 
> > Changes since v1:
> > - New patch.
> > ---
> >  include/uapi/linux/landlock.h                |  2 +
> >  security/landlock/errata.h                   | 87 ++++++++++++++++++++
> >  security/landlock/setup.c                    | 30 +++++++
> >  security/landlock/setup.h                    |  3 +
> >  security/landlock/syscalls.c                 | 22 ++++-
> >  tools/testing/selftests/landlock/base_test.c | 38 ++++++++-
> >  6 files changed, 177 insertions(+), 5 deletions(-)
> >  create mode 100644 security/landlock/errata.h
> > 
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index e1d2c27533b4..8806a132d7b8 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -57,9 +57,11 @@ struct landlock_ruleset_attr {
> >   *
> >   * - %LANDLOCK_CREATE_RULESET_VERSION: Get the highest supported Landlock ABI
> >   *   version.
> > + * - %LANDLOCK_CREATE_RULESET_ERRATA: Get a bitmask of fixed issues.
> >   */
> >  /* clang-format off */
> >  #define LANDLOCK_CREATE_RULESET_VERSION			(1U << 0)
> > +#define LANDLOCK_CREATE_RULESET_ERRATA			(1U << 1)
> >  /* clang-format on */
> >  
> >  /**
> > diff --git a/security/landlock/errata.h b/security/landlock/errata.h
> > new file mode 100644
> > index 000000000000..f26b28b9873d
> > --- /dev/null
> > +++ b/security/landlock/errata.h
> > @@ -0,0 +1,87 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Landlock - Errata information
> > + *
> > + * Copyright © 2025 Microsoft Corporation
> > + */
> > +
> > +#ifndef _SECURITY_LANDLOCK_ERRATA_H
> > +#define _SECURITY_LANDLOCK_ERRATA_H
> > +
> > +#include <linux/init.h>
> > +
> > +struct landlock_erratum {
> > +	const int abi;
> > +	const u8 number;
> > +};
> > +
> > +/* clang-format off */
> > +#define LANDLOCK_ERRATUM(NUMBER) \
> > +	{ \
> > +		.abi = LANDLOCK_ERRATA_ABI, \
> > +		.number = NUMBER, \
> > +	},
> > +/* clang-format on */
> > +
> > +/*
> > + * Some fixes may require user space to check if they are applied on the running
> > + * kernel before using a specific feature.  For instance, this applies when a
> > + * restriction was previously too restrictive and is now getting relaxed (for
> > + * compatibility or semantic reasons).  However, non-visible changes for
> > + * legitimate use (e.g. security fixes) do not require an erratum.
> > + */
> > +static const struct landlock_erratum landlock_errata_init[] __initconst = {
> > +
> > +/*
> > + * Only Sparse may not implement __has_include.  If a compiler does not
> > + * implement __has_include, a warning will be printed at boot time (see
> > + * setup.c).
> > + */
> > +#ifdef __has_include
> > +
> > +#define LANDLOCK_ERRATA_ABI 1
> > +#if __has_include("errata/abi-1.h")
> > +#include "errata/abi-1.h"
> > +#endif
> > +#undef LANDLOCK_ERRATA_ABI
> > +
> > +#define LANDLOCK_ERRATA_ABI 2
> > +#if __has_include("errata/abi-2.h")
> > +#include "errata/abi-2.h"
> > +#endif
> > +#undef LANDLOCK_ERRATA_ABI
> > +
> > +#define LANDLOCK_ERRATA_ABI 3
> > +#if __has_include("errata/abi-3.h")
> > +#include "errata/abi-3.h"
> > +#endif
> > +#undef LANDLOCK_ERRATA_ABI
> > +
> > +#define LANDLOCK_ERRATA_ABI 4
> > +#if __has_include("errata/abi-4.h")
> > +#include "errata/abi-4.h"
> > +#endif
> > +#undef LANDLOCK_ERRATA_ABI
> 
> 
> > +
> > +/*
> > + * For each new erratum, we need to include all the ABI files up to the impacted
> > + * ABI to make all potential future intermediate errata easy to backport.
> > + *
> > + * If such change involves more than one ABI addition, then it must be in a
> > + * dedicated commit with the same Fixes tag as used for the actual fix.
> > + *
> > + * Each commit creating a new security/landlock/errata/abi-*.h file must have a
> > + * Depends-on tag to reference the commit that previously added the line to
> > + * include this new file, except if the original Fixes tag is enough.
> > + *
> > + * Each erratum must be documented in its related ABI file, and a dedicated
> > + * commit must update Documentation/userspace-api/landlock.rst to include this
> > + * erratum.  This commit will not be backported.
> > + */
> > +
> > +#endif
> > +
> > +	{}
> > +};
> > +
> > +#endif /* _SECURITY_LANDLOCK_ERRATA_H */
> > diff --git a/security/landlock/setup.c b/security/landlock/setup.c
> > index c71832a8e369..0c85ea27e409 100644
> > --- a/security/landlock/setup.c
> > +++ b/security/landlock/setup.c
> > @@ -6,12 +6,14 @@
> >   * Copyright © 2018-2020 ANSSI
> >   */
> >  
> > +#include <linux/bits.h>
> >  #include <linux/init.h>
> >  #include <linux/lsm_hooks.h>
> >  #include <uapi/linux/lsm.h>
> >  
> >  #include "common.h"
> >  #include "cred.h"
> > +#include "errata.h"
> >  #include "fs.h"
> >  #include "net.h"
> >  #include "setup.h"
> > @@ -31,8 +33,36 @@ struct lsm_blob_sizes landlock_blob_sizes __ro_after_init = {
> >  	.lbs_superblock = sizeof(struct landlock_superblock_security),
> >  };
> >  
> > +int landlock_errata __ro_after_init;
> > +
> > +static void __init compute_errata(void)
> > +{
> > +	size_t i;
> > +
> > +#ifndef __has_include
> > +	/*
> > +	 * This is a safeguard to make sure the compiler implements
> > +	 * __has_include (see errata.h).
> > +	 */
> > +	WARN_ON_ONCE(1);
> > +	return;
> > +#endif
> > +
> > +	for (i = 0; landlock_errata_init[i].number; i++) {
> > +		const int prev_errata = landlock_errata;
> > +
> > +		if (WARN_ON_ONCE(landlock_errata_init[i].abi >
> > +				 landlock_abi_version))
> > +			continue;
> 
> IIUC, if we hit this condition, someone has tried to backport an
> erratum that does not apply here?

Yes, it's just a safeguard to ensure consistency.

> Shouldn't this ideally be a compile
> time error?

Yes, that would be nice but I didn't find a way to do it.

> Then downstream kernel maintainers would notice it
> earlier when they apply the wrong patch?

This check is mainly for downstream kernels yes, but that's also useful
for us.

> 
> Can this scenario really happen?  It feels that this should normally
> already be caught in merge conflicts at cherry picking time?

Well, a lot of things can happen when backporting *features* to older
kernels.  Anyway, it's a cheap check.

> 
> 
> > +
> > +		landlock_errata |= BIT(landlock_errata_init[i].number - 1);
> > +		WARN_ON_ONCE(prev_errata == landlock_errata);
> > +	}
> > +}
> > +
> >  static int __init landlock_init(void)
> >  {
> > +	compute_errata();
> >  	landlock_add_cred_hooks();
> >  	landlock_add_task_hooks();
> >  	landlock_add_fs_hooks();
> > diff --git a/security/landlock/setup.h b/security/landlock/setup.h
> > index c4252d46d49d..fca307c35fee 100644
> > --- a/security/landlock/setup.h
> > +++ b/security/landlock/setup.h
> > @@ -11,7 +11,10 @@
> >  
> >  #include <linux/lsm_hooks.h>
> >  
> > +extern const int landlock_abi_version;
> > +
> >  extern bool landlock_initialized;
> > +extern int landlock_errata;
> >  
> >  extern struct lsm_blob_sizes landlock_blob_sizes;
> >  extern const struct lsm_id landlock_lsmid;
> > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> > index a9760d252fc2..cf9e0483e542 100644
> > --- a/security/landlock/syscalls.c
> > +++ b/security/landlock/syscalls.c
> > @@ -160,7 +160,9 @@ static const struct file_operations ruleset_fops = {
> >   *        the new ruleset.
> >   * @size: Size of the pointed &struct landlock_ruleset_attr (needed for
> >   *        backward and forward compatibility).
> > - * @flags: Supported value: %LANDLOCK_CREATE_RULESET_VERSION.
> > + * @flags: Supported value:
> > + *         - %LANDLOCK_CREATE_RULESET_VERSION
> > + *         - %LANDLOCK_CREATE_RULESET_ERRATA
> >   *
> >   * This system call enables to create a new Landlock ruleset, and returns the
> >   * related file descriptor on success.
> > @@ -169,6 +171,10 @@ static const struct file_operations ruleset_fops = {
> >   * 0, then the returned value is the highest supported Landlock ABI version
> >   * (starting at 1).
> >   *
> > + * If @flags is %LANDLOCK_CREATE_RULESET_ERRATA and @attr is NULL and @size is
> > + * 0, then the returned value is a bitmask of fixed issues for the current
> > + * Landlock ABI version.
> 
> We should probably say here where that list is defined.

It's defined in the user space documentation (see commits adding the
errata).  BTW, I'll send a patch to improve the documentation (mostly
cosmetic).

> 
> Should the errata numbers also be constants in the
> uapi/linux/landlock.h header?

As explained above, that would mean that we don't use these constants in
the kernel, which would be weird.

> 
> > + *
> >   * Possible returned errors are:
> >   *
> >   * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
> > @@ -192,9 +198,15 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> >  		return -EOPNOTSUPP;
> >  
> >  	if (flags) {
> > -		if ((flags == LANDLOCK_CREATE_RULESET_VERSION) && !attr &&
> > -		    !size)
> > -			return LANDLOCK_ABI_VERSION;
> > +		if (attr || size)
> > +			return -EINVAL;
> > +
> > +		if (flags == LANDLOCK_CREATE_RULESET_VERSION)
> > +			return landlock_abi_version;
> > +
> > +		if (flags == LANDLOCK_CREATE_RULESET_ERRATA)
> > +			return landlock_errata;
> > +
> >  		return -EINVAL;
> >  	}
> >  
> > @@ -235,6 +247,8 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> >  	return ruleset_fd;
> >  }
> >  
> > +const int landlock_abi_version = LANDLOCK_ABI_VERSION;
> > +
> >  /*
> >   * Returns an owned ruleset from a FD. It is thus needed to call
> >   * landlock_put_ruleset() on the return value.
> > diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> > index 1bc16fde2e8a..c0abadd0bbbe 100644
> > --- a/tools/testing/selftests/landlock/base_test.c
> > +++ b/tools/testing/selftests/landlock/base_test.c
> > @@ -98,10 +98,46 @@ TEST(abi_version)
> >  	ASSERT_EQ(EINVAL, errno);
> >  }
> >  
> > +TEST(errata)
> > +{
> > +	const struct landlock_ruleset_attr ruleset_attr = {
> > +		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
> > +	};
> > +	int errata;
> > +
> > +	errata = landlock_create_ruleset(NULL, 0,
> > +					 LANDLOCK_CREATE_RULESET_ERRATA);
> > +	/* The errata bitmask will not be backported to tests. */
> > +	ASSERT_LE(0, errata);
> > +	TH_LOG("errata: 0x%x", errata);
> > +
> > +	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> > +					      LANDLOCK_CREATE_RULESET_ERRATA));
> > +	ASSERT_EQ(EINVAL, errno);
> > +
> > +	ASSERT_EQ(-1, landlock_create_ruleset(NULL, sizeof(ruleset_attr),
> > +					      LANDLOCK_CREATE_RULESET_ERRATA));
> > +	ASSERT_EQ(EINVAL, errno);
> > +
> > +	ASSERT_EQ(-1,
> > +		  landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr),
> > +					  LANDLOCK_CREATE_RULESET_ERRATA));
> > +	ASSERT_EQ(EINVAL, errno);
> > +
> > +	ASSERT_EQ(-1, landlock_create_ruleset(
> > +			      NULL, 0,
> > +			      LANDLOCK_CREATE_RULESET_VERSION |
> > +				      LANDLOCK_CREATE_RULESET_ERRATA));
> > +	ASSERT_EQ(-1, landlock_create_ruleset(NULL, 0,
> > +					      LANDLOCK_CREATE_RULESET_ERRATA |
> > +						      1 << 31));
> > +	ASSERT_EQ(EINVAL, errno);
> 
> There are two calls to landlock_create_ruleset() here, but only one
> ASSERT_EQ(EINVAL, errno).

Correct, I'll add another errno check.

> I do not understand why the second call to
> landlock_create_ruleset() was done here (with the | 1 << 31) -- was
> that left here by accident?

This second check is to make sure that an unknown flag results to an
EINVAL error.  A similar check is done with
LANDLOCK_CREATE_RULESET_VERSION.

> 
> 
> > +}
> > +
> >  /* Tests ordering of syscall argument checks. */
> >  TEST(create_ruleset_checks_ordering)
> >  {
> > -	const int last_flag = LANDLOCK_CREATE_RULESET_VERSION;
> > +	const int last_flag = LANDLOCK_CREATE_RULESET_ERRATA;
> >  	const int invalid_flag = last_flag << 1;
> >  	int ruleset_fd;
> >  	const struct landlock_ruleset_attr ruleset_attr = {
> > -- 
> > 2.48.1
> > 
> 
> –Günther
> 

[PATCH v2 3/8] landlock: Add erratum for TCP fix

[Mickaël Salaün]

Add erratum for the TCP socket identification fixed with commit
854277e2cc8c ("landlock: Fix non-TCP sockets restriction").

Fixes: 854277e2cc8c ("landlock: Fix non-TCP sockets restriction")
Cc: Günther Noack <gnoack@google.com>
Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250318161443.279194-4-mic@digikod.net
---

Changes since v1:
- New patch.
---
 security/landlock/errata/abi-4.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 security/landlock/errata/abi-4.h

diff --git a/security/landlock/errata/abi-4.h b/security/landlock/errata/abi-4.h
new file mode 100644
index 000000000000..c052ee54f89f
--- /dev/null
+++ b/security/landlock/errata/abi-4.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/**
+ * DOC: erratum_1
+ *
+ * Erratum 1: TCP socket identification
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This fix addresses an issue where IPv4 and IPv6 stream sockets (e.g., SMC,
+ * MPTCP, or SCTP) were incorrectly restricted by TCP access rights during
+ * :manpage:`bind(2)` and :manpage:`connect(2)` operations. This change ensures
+ * that only TCP sockets are subject to TCP access rights, allowing other
+ * protocols to operate without unnecessary restrictions.
+ */
+LANDLOCK_ERRATUM(1)
-- 
2.48.1

[PATCH v2 4/8] landlock: Prepare to add second errata

[Mickaël Salaün]

Potentially include errata for Landlock ABI v5 (Linux 6.10) and v6
(Linux 6.12).  That will be useful for the following signal scoping
erratum.

As explained in errata.h, this commit should be backportable without
conflict down to ABI v5.  It must then not include the errata/abi-6.h
file.

Fixes: 54a6e6bbf3be ("landlock: Add signal scoping")
Cc: Günther Noack <gnoack@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250318161443.279194-5-mic@digikod.net
---

Changes since v1:
- New patch.
---
 security/landlock/errata.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/security/landlock/errata.h b/security/landlock/errata.h
index f26b28b9873d..8e626accac10 100644
--- a/security/landlock/errata.h
+++ b/security/landlock/errata.h
@@ -63,6 +63,18 @@ static const struct landlock_erratum landlock_errata_init[] __initconst = {
 #endif
 #undef LANDLOCK_ERRATA_ABI
 
+#define LANDLOCK_ERRATA_ABI 5
+#if __has_include("errata/abi-5.h")
+#include "errata/abi-5.h"
+#endif
+#undef LANDLOCK_ERRATA_ABI
+
+#define LANDLOCK_ERRATA_ABI 6
+#if __has_include("errata/abi-6.h")
+#include "errata/abi-6.h"
+#endif
+#undef LANDLOCK_ERRATA_ABI
+
 /*
  * For each new erratum, we need to include all the ABI files up to the impacted
  * ABI to make all potential future intermediate errata easy to backport.
-- 
2.48.1

[PATCH v2 5/8] landlock: Always allow signals between threads of the same process

[Mickaël Salaün]

Because Linux credentials are managed per thread, user space relies on
some hack to synchronize credential update across threads from the same
process.  This is required by the Native POSIX Threads Library and
implemented by set*id(2) wrappers and libcap(3) to use tgkill(2) to
synchronize threads.  See nptl(7) and libpsx(3).  Furthermore, some
runtimes like Go do not enable developers to have control over threads
[1].

To avoid potential issues, and because threads are not security
boundaries, let's relax the Landlock (optional) signal scoping to always
allow signals sent between threads of the same process.  This exception
is similar to the __ptrace_may_access() one.

hook_file_set_fowner() now checks if the target task is part of the same
process as the caller.  If this is the case, then the related signal
triggered by the socket will always be allowed.

Scoping of abstract UNIX sockets is not changed because kernel objects
(e.g. sockets) should be tied to their creator's domain at creation
time.

Note that creating one Landlock domain per thread puts each of these
threads (and their future children) in their own scope, which is
probably not what users expect, especially in Go where we do not control
threads.  However, being able to drop permissions on all threads should
not be restricted by signal scoping.  We are working on a way to make it
possible to atomically restrict all threads of a process with the same
domain [2].

Add erratum for signal scoping.

Closes: https://github.com/landlock-lsm/go-landlock/issues/36
Fixes: 54a6e6bbf3be ("landlock: Add signal scoping")
Fixes: c8994965013e ("selftests/landlock: Test signal scoping for threads")
Depends-on: 26f204380a3c ("fs: Fix file_set_fowner LSM hook inconsistencies")
Link: https://pkg.go.dev/kernel.org/pub/linux/libs/security/libcap/psx [1]
Link: https://github.com/landlock-lsm/linux/issues/2 [2]
Cc: Günther Noack <gnoack@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Tahera Fahimi <fahimitahera@gmail.com>
Cc: stable@vger.kernel.org
Acked-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250318161443.279194-6-mic@digikod.net
---

Changes since v1:
- Add Acked-by Christian.
- Add Landlock erratum.
- Update subject.
---
 security/landlock/errata/abi-6.h              | 19 ++++++++++++++++
 security/landlock/fs.c                        | 22 +++++++++++++++----
 security/landlock/task.c                      | 12 ++++++++++
 .../selftests/landlock/scoped_signal_test.c   |  2 +-
 4 files changed, 50 insertions(+), 5 deletions(-)
 create mode 100644 security/landlock/errata/abi-6.h

diff --git a/security/landlock/errata/abi-6.h b/security/landlock/errata/abi-6.h
new file mode 100644
index 000000000000..df7bc0e1fdf4
--- /dev/null
+++ b/security/landlock/errata/abi-6.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/**
+ * DOC: erratum_2
+ *
+ * Erratum 2: Scoped signal handling
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This fix addresses an issue where signal scoping was overly restrictive,
+ * preventing sandboxed threads from signaling other threads within the same
+ * process if they belonged to different domains.  Because threads are not
+ * security boundaries, user space might assume that any thread within the same
+ * process can send signals between themselves (see :manpage:`nptl(7)` and
+ * :manpage:`libpsx(3)`).  Consistent with :manpage:`ptrace(2)` behavior, direct
+ * interaction between threads of the same process should always be allowed.
+ * This change ensures that any thread is allowed to send signals to any other
+ * thread within the same process, regardless of their domain.
+ */
+LANDLOCK_ERRATUM(2)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 71b9dc331aae..47c862fe14e4 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -27,7 +27,9 @@
 #include <linux/mount.h>
 #include <linux/namei.h>
 #include <linux/path.h>
+#include <linux/pid.h>
 #include <linux/rcupdate.h>
+#include <linux/sched/signal.h>
 #include <linux/spinlock.h>
 #include <linux/stat.h>
 #include <linux/types.h>
@@ -1630,15 +1632,27 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
 
 static void hook_file_set_fowner(struct file *file)
 {
-	struct landlock_ruleset *new_dom, *prev_dom;
+	struct fown_struct *fown = file_f_owner(file);
+	struct landlock_ruleset *new_dom = NULL;
+	struct landlock_ruleset *prev_dom;
+	struct task_struct *p;
 
 	/*
 	 * Lock already held by __f_setown(), see commit 26f204380a3c ("fs: Fix
 	 * file_set_fowner LSM hook inconsistencies").
 	 */
-	lockdep_assert_held(&file_f_owner(file)->lock);
-	new_dom = landlock_get_current_domain();
-	landlock_get_ruleset(new_dom);
+	lockdep_assert_held(&fown->lock);
+
+	/*
+	 * Always allow sending signals between threads of the same process.  This
+	 * ensures consistency with hook_task_kill().
+	 */
+	p = pid_task(fown->pid, fown->pid_type);
+	if (!same_thread_group(p, current)) {
+		new_dom = landlock_get_current_domain();
+		landlock_get_ruleset(new_dom);
+	}
+
 	prev_dom = landlock_file(file)->fown_domain;
 	landlock_file(file)->fown_domain = new_dom;
 
diff --git a/security/landlock/task.c b/security/landlock/task.c
index dc7dab78392e..4578ce6e319d 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c
@@ -13,6 +13,7 @@
 #include <linux/lsm_hooks.h>
 #include <linux/rcupdate.h>
 #include <linux/sched.h>
+#include <linux/sched/signal.h>
 #include <net/af_unix.h>
 #include <net/sock.h>
 
@@ -264,6 +265,17 @@ static int hook_task_kill(struct task_struct *const p,
 		/* Dealing with USB IO. */
 		dom = landlock_cred(cred)->domain;
 	} else {
+		/*
+		 * Always allow sending signals between threads of the same process.
+		 * This is required for process credential changes by the Native POSIX
+		 * Threads Library and implemented by the set*id(2) wrappers and
+		 * libcap(3) with tgkill(2).  See nptl(7) and libpsx(3).
+		 *
+		 * This exception is similar to the __ptrace_may_access() one.
+		 */
+		if (same_thread_group(p, current))
+			return 0;
+
 		dom = landlock_get_current_domain();
 	}
 	dom = landlock_get_applicable_domain(dom, signal_scope);
diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
index 475ee62a832d..767f117703b7 100644
--- a/tools/testing/selftests/landlock/scoped_signal_test.c
+++ b/tools/testing/selftests/landlock/scoped_signal_test.c
@@ -281,7 +281,7 @@ TEST(signal_scoping_threads)
 	/* Restricts the domain after creating the first thread. */
 	create_scoped_domain(_metadata, LANDLOCK_SCOPE_SIGNAL);
 
-	ASSERT_EQ(EPERM, pthread_kill(no_sandbox_thread, 0));
+	ASSERT_EQ(0, pthread_kill(no_sandbox_thread, 0));
 	ASSERT_EQ(1, write(thread_pipe[1], ".", 1));
 
 	ASSERT_EQ(0, pthread_create(&scoped_thread, NULL, thread_func, NULL));
-- 
2.48.1

Re: [PATCH v2 5/8] landlock: Always allow signals between threads of the same process

[Mickaël Salaün]

On Tue, Mar 18, 2025 at 05:14:40PM +0100, Mickaël Salaün wrote:
> Because Linux credentials are managed per thread, user space relies on
> some hack to synchronize credential update across threads from the same
> process.  This is required by the Native POSIX Threads Library and
> implemented by set*id(2) wrappers and libcap(3) to use tgkill(2) to
> synchronize threads.  See nptl(7) and libpsx(3).  Furthermore, some
> runtimes like Go do not enable developers to have control over threads
> [1].
> 
> To avoid potential issues, and because threads are not security
> boundaries, let's relax the Landlock (optional) signal scoping to always
> allow signals sent between threads of the same process.  This exception
> is similar to the __ptrace_may_access() one.
> 
> hook_file_set_fowner() now checks if the target task is part of the same
> process as the caller.  If this is the case, then the related signal
> triggered by the socket will always be allowed.
> 
> Scoping of abstract UNIX sockets is not changed because kernel objects
> (e.g. sockets) should be tied to their creator's domain at creation
> time.
> 
> Note that creating one Landlock domain per thread puts each of these
> threads (and their future children) in their own scope, which is
> probably not what users expect, especially in Go where we do not control
> threads.  However, being able to drop permissions on all threads should
> not be restricted by signal scoping.  We are working on a way to make it
> possible to atomically restrict all threads of a process with the same
> domain [2].
> 
> Add erratum for signal scoping.
> 
> Closes: https://github.com/landlock-lsm/go-landlock/issues/36
> Fixes: 54a6e6bbf3be ("landlock: Add signal scoping")
> Fixes: c8994965013e ("selftests/landlock: Test signal scoping for threads")
> Depends-on: 26f204380a3c ("fs: Fix file_set_fowner LSM hook inconsistencies")
> Link: https://pkg.go.dev/kernel.org/pub/linux/libs/security/libcap/psx [1]
> Link: https://github.com/landlock-lsm/linux/issues/2 [2]
> Cc: Günther Noack <gnoack@google.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Tahera Fahimi <fahimitahera@gmail.com>
> Cc: stable@vger.kernel.org
> Acked-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20250318161443.279194-6-mic@digikod.net

> index 71b9dc331aae..47c862fe14e4 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -27,7 +27,9 @@
>  #include <linux/mount.h>
>  #include <linux/namei.h>
>  #include <linux/path.h>
> +#include <linux/pid.h>
>  #include <linux/rcupdate.h>
> +#include <linux/sched/signal.h>
>  #include <linux/spinlock.h>
>  #include <linux/stat.h>
>  #include <linux/types.h>
> @@ -1630,15 +1632,27 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
>  
>  static void hook_file_set_fowner(struct file *file)
>  {
> -	struct landlock_ruleset *new_dom, *prev_dom;
> +	struct fown_struct *fown = file_f_owner(file);
> +	struct landlock_ruleset *new_dom = NULL;
> +	struct landlock_ruleset *prev_dom;
> +	struct task_struct *p;
>  
>  	/*
>  	 * Lock already held by __f_setown(), see commit 26f204380a3c ("fs: Fix
>  	 * file_set_fowner LSM hook inconsistencies").
>  	 */
> -	lockdep_assert_held(&file_f_owner(file)->lock);
> -	new_dom = landlock_get_current_domain();
> -	landlock_get_ruleset(new_dom);
> +	lockdep_assert_held(&fown->lock);
> +
> +	/*
> +	 * Always allow sending signals between threads of the same process.  This
> +	 * ensures consistency with hook_task_kill().
> +	 */
> +	p = pid_task(fown->pid, fown->pid_type);
> +	if (!same_thread_group(p, current)) {

There is a missing pointer check.  I'll apply this:

-       if (!same_thread_group(p, current)) {
+       if (!p || !same_thread_group(p, current)) {

> +		new_dom = landlock_get_current_domain();
> +		landlock_get_ruleset(new_dom);
> +	}
> +
>  	prev_dom = landlock_file(file)->fown_domain;
>  	landlock_file(file)->fown_domain = new_dom;
>  

Re: [PATCH v2 5/8] landlock: Always allow signals between threads of the same process

[kernel test robot]

Hello,

kernel test robot noticed "Oops:general_protection_fault,probably_for_non-canonical_address#:#[##]SMP_KASAN" on:

commit: b9fb5bfdb361fd6d945c09c93d351740310a26c7 ("[PATCH v2 5/8] landlock: Always allow signals between threads of the same process")
url: https://github.com/intel-lab-lkp/linux/commits/Micka-l-Sala-n/landlock-Move-code-to-ease-future-backports/20250319-003737
patch link: https://lore.kernel.org/all/20250318161443.279194-6-mic@digikod.net/
patch subject: [PATCH v2 5/8] landlock: Always allow signals between threads of the same process

in testcase: trinity
version: trinity-x86_64-ba2360ed-1_20241228
with following parameters:

	runtime: 300s
	group: group-03
	nr_groups: 5



config: x86_64-randconfig-005-20250325
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


we noticed the issue happens randomly (35 times out of 200 runs as below).
but parent keeps clean.


37897789c51dd898 b9fb5bfdb361fd6d945c09c93d3
---------------- ---------------------------
       fail:runs  %reproduction    fail:runs
           |             |             |
           :200         18%          35:200   dmesg.KASAN:null-ptr-deref_in_range[#-#]
           :200         18%          35:200   dmesg.Kernel_panic-not_syncing:Fatal_exception
           :200         18%          35:200   dmesg.Oops:general_protection_fault,probably_for_non-canonical_address#:#[##]SMP_KASAN
           :200         18%          35:200   dmesg.RIP:hook_file_set_fowner



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202503261534.22d970e8-lkp@intel.com


[  354.738531][  T222]
[  355.199494][  T222] [main] 2245715 iterations. [F:1644455 S:601688 HI:11581]
[  355.199514][  T222]
[  355.934630][  T222] [main] 2273938 iterations. [F:1665198 S:609188 HI:11581]
[  355.934650][  T222]
[  356.308897][ T3147] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000151: 0000 [#1] SMP KASAN
[  356.309510][ T3147] KASAN: null-ptr-deref in range [0x0000000000000a88-0x0000000000000a8f]
[  356.309910][ T3147] CPU: 1 UID: 65534 PID: 3147 Comm: trinity-c2 Not tainted 6.14.0-rc5-00005-gb9fb5bfdb361 #1 145c38dc5407add8933da653ccf9cf31d58da93c
[  356.310560][ T3147] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 356.311050][ T3147] RIP: 0010:hook_file_set_fowner (kbuild/src/consumer/include/linux/sched/signal.h:707 (discriminator 9) kbuild/src/consumer/security/landlock/fs.c:1651 (discriminator 9)) 
[ 356.311345][ T3147] Code: 49 8b 7c 24 50 65 4c 8b 25 e7 e4 0c 7e e8 52 63 33 ff 48 ba 00 00 00 00 00 fc ff df 48 8d b8 88 0a 00 00 48 89 f9 48 c1 e9 03 <80> 3c 11 00 0f 85 7e 02 00 00 49 8d bc 24 88 0a 00 00 4c 8b a8 88
All code
========
   0:	49 8b 7c 24 50       	mov    0x50(%r12),%rdi
   5:	65 4c 8b 25 e7 e4 0c 	mov    %gs:0x7e0ce4e7(%rip),%r12        # 0x7e0ce4f4
   c:	7e 
   d:	e8 52 63 33 ff       	call   0xffffffffff336364
  12:	48 ba 00 00 00 00 00 	movabs $0xdffffc0000000000,%rdx
  19:	fc ff df 
  1c:	48 8d b8 88 0a 00 00 	lea    0xa88(%rax),%rdi
  23:	48 89 f9             	mov    %rdi,%rcx
  26:	48 c1 e9 03          	shr    $0x3,%rcx
  2a:*	80 3c 11 00          	cmpb   $0x0,(%rcx,%rdx,1)		<-- trapping instruction
  2e:	0f 85 7e 02 00 00    	jne    0x2b2
  34:	49 8d bc 24 88 0a 00 	lea    0xa88(%r12),%rdi
  3b:	00 
  3c:	4c                   	rex.WR
  3d:	8b                   	.byte 0x8b
  3e:	a8 88                	test   $0x88,%al

Code starting with the faulting instruction
===========================================
   0:	80 3c 11 00          	cmpb   $0x0,(%rcx,%rdx,1)
   4:	0f 85 7e 02 00 00    	jne    0x288
   a:	49 8d bc 24 88 0a 00 	lea    0xa88(%r12),%rdi
  11:	00 
  12:	4c                   	rex.WR
  13:	8b                   	.byte 0x8b
  14:	a8 88                	test   $0x88,%al
[  356.312254][ T3147] RSP: 0018:ffffc9000883fd20 EFLAGS: 00010002
[  356.312556][ T3147] RAX: 0000000000000000 RBX: ffff88816ee4c700 RCX: 0000000000000151
[  356.312933][ T3147] RDX: dffffc0000000000 RSI: 0000000000000000 RDI: 0000000000000a88
[  356.313310][ T3147] RBP: ffffc9000883fd48 R08: 0000000000000000 R09: 0000000000000000
[  356.313687][ T3147] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88814f0c8000
[  356.314063][ T3147] R13: ffff88814f92b700 R14: ffff888161e71450 R15: ffff888161e71408
[  356.314440][ T3147] FS:  00007f3c72136740(0000) GS:ffff8883af000000(0000) knlGS:0000000000000000
[  356.314879][ T3147] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  356.315194][ T3147] CR2: 00007f3c708bd000 CR3: 0000000165606000 CR4: 00000000000406f0
[  356.315573][ T3147] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  356.315950][ T3147] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  356.316334][ T3147] Call Trace:
[  356.316498][ T3147]  <TASK>
[ 356.316645][ T3147] ? show_regs (kbuild/src/consumer/arch/x86/kernel/dumpstack.c:479) 
[ 356.316859][ T3147] ? die_addr (kbuild/src/consumer/arch/x86/kernel/dumpstack.c:421 kbuild/src/consumer/arch/x86/kernel/dumpstack.c:460) 
[ 356.317066][ T3147] ? exc_general_protection (kbuild/src/consumer/arch/x86/kernel/traps.c:751 kbuild/src/consumer/arch/x86/kernel/traps.c:693) 
[ 356.317349][ T3147] ? asm_exc_general_protection (kbuild/src/consumer/arch/x86/include/asm/idtentry.h:574) 


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250326/202503261534.22d970e8-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 5/8] landlock: Always allow signals between threads of the same process

[Mickaël Salaün]

Thanks for the report.

I assumed __f_setown() was only called in an RCU read-side critical section but
that's not the case (e.g. fcntl_dirnotify).  I moved the pid_task() call in a
dedicated function to protect it with an RCU guard.  Here is the new hunk:


@@ -1628,21 +1630,46 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
        return -EACCES;
 }

-static void hook_file_set_fowner(struct file *file)
+/*
+ * Always allow sending signals between threads of the same process.  This
+ * ensures consistency with hook_task_kill().
+ */
+static bool control_current_fowner(struct fown_struct *const fown)
 {
-       struct landlock_ruleset *new_dom, *prev_dom;
+       struct task_struct *p;

        /*
         * Lock already held by __f_setown(), see commit 26f204380a3c ("fs: Fix
         * file_set_fowner LSM hook inconsistencies").
         */
-       lockdep_assert_held(&file_f_owner(file)->lock);
-       new_dom = landlock_get_current_domain();
-       landlock_get_ruleset(new_dom);
+       lockdep_assert_held(&fown->lock);
+
+       /*
+        * Some callers (e.g. fcntl_dirnotify) may not be in an RCU read-side
+        * critical section.
+        */
+       guard(rcu)();
+       p = pid_task(fown->pid, fown->pid_type);
+       if (!p)
+               return true;
+
+       return !same_thread_group(p, current);
+}
+
+static void hook_file_set_fowner(struct file *file)
+{
+       struct landlock_ruleset *prev_dom;
+       struct landlock_ruleset *new_dom = NULL;
+
+       if (control_current_fowner(file_f_owner(file))) {
+               new_dom = landlock_get_current_domain();
+               landlock_get_ruleset(new_dom);
+       }
+
        prev_dom = landlock_file(file)->fown_domain;
        landlock_file(file)->fown_domain = new_dom;

-       /* Called in an RCU read-side critical section. */
+       /* May be called in an RCU read-side critical section. */
        landlock_put_ruleset_deferred(prev_dom);
 }


This other report gives more details:
https://lore.kernel.org/all/202503261510.f9652c11-lkp@intel.com/


On Wed, Mar 26, 2025 at 03:51:50PM +0800, kernel test robot wrote:
> 
> 
> Hello,
> 
> kernel test robot noticed "Oops:general_protection_fault,probably_for_non-canonical_address#:#[##]SMP_KASAN" on:
> 
> commit: b9fb5bfdb361fd6d945c09c93d351740310a26c7 ("[PATCH v2 5/8] landlock: Always allow signals between threads of the same process")
> url: https://github.com/intel-lab-lkp/linux/commits/Micka-l-Sala-n/landlock-Move-code-to-ease-future-backports/20250319-003737
> patch link: https://lore.kernel.org/all/20250318161443.279194-6-mic@digikod.net/
> patch subject: [PATCH v2 5/8] landlock: Always allow signals between threads of the same process
> 
> in testcase: trinity
> version: trinity-x86_64-ba2360ed-1_20241228
> with following parameters:
> 
> 	runtime: 300s
> 	group: group-03
> 	nr_groups: 5
> 
> 
> 
> config: x86_64-randconfig-005-20250325
> compiler: gcc-12
> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
> 
> (please refer to attached dmesg/kmsg for entire log/backtrace)
> 
> 
> we noticed the issue happens randomly (35 times out of 200 runs as below).
> but parent keeps clean.
> 
> 
> 37897789c51dd898 b9fb5bfdb361fd6d945c09c93d3
> ---------------- ---------------------------
>        fail:runs  %reproduction    fail:runs
>            |             |             |
>            :200         18%          35:200   dmesg.KASAN:null-ptr-deref_in_range[#-#]
>            :200         18%          35:200   dmesg.Kernel_panic-not_syncing:Fatal_exception
>            :200         18%          35:200   dmesg.Oops:general_protection_fault,probably_for_non-canonical_address#:#[##]SMP_KASAN
>            :200         18%          35:200   dmesg.RIP:hook_file_set_fowner
> 
> 
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <oliver.sang@intel.com>
> | Closes: https://lore.kernel.org/oe-lkp/202503261534.22d970e8-lkp@intel.com
> 
> 
> [  354.738531][  T222]
> [  355.199494][  T222] [main] 2245715 iterations. [F:1644455 S:601688 HI:11581]
> [  355.199514][  T222]
> [  355.934630][  T222] [main] 2273938 iterations. [F:1665198 S:609188 HI:11581]
> [  355.934650][  T222]
> [  356.308897][ T3147] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000151: 0000 [#1] SMP KASAN
> [  356.309510][ T3147] KASAN: null-ptr-deref in range [0x0000000000000a88-0x0000000000000a8f]
> [  356.309910][ T3147] CPU: 1 UID: 65534 PID: 3147 Comm: trinity-c2 Not tainted 6.14.0-rc5-00005-gb9fb5bfdb361 #1 145c38dc5407add8933da653ccf9cf31d58da93c
> [  356.310560][ T3147] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> [ 356.311050][ T3147] RIP: 0010:hook_file_set_fowner (kbuild/src/consumer/include/linux/sched/signal.h:707 (discriminator 9) kbuild/src/consumer/security/landlock/fs.c:1651 (discriminator 9)) 
> [ 356.311345][ T3147] Code: 49 8b 7c 24 50 65 4c 8b 25 e7 e4 0c 7e e8 52 63 33 ff 48 ba 00 00 00 00 00 fc ff df 48 8d b8 88 0a 00 00 48 89 f9 48 c1 e9 03 <80> 3c 11 00 0f 85 7e 02 00 00 49 8d bc 24 88 0a 00 00 4c 8b a8 88
> All code
> ========
>    0:	49 8b 7c 24 50       	mov    0x50(%r12),%rdi
>    5:	65 4c 8b 25 e7 e4 0c 	mov    %gs:0x7e0ce4e7(%rip),%r12        # 0x7e0ce4f4
>    c:	7e 
>    d:	e8 52 63 33 ff       	call   0xffffffffff336364
>   12:	48 ba 00 00 00 00 00 	movabs $0xdffffc0000000000,%rdx
>   19:	fc ff df 
>   1c:	48 8d b8 88 0a 00 00 	lea    0xa88(%rax),%rdi
>   23:	48 89 f9             	mov    %rdi,%rcx
>   26:	48 c1 e9 03          	shr    $0x3,%rcx
>   2a:*	80 3c 11 00          	cmpb   $0x0,(%rcx,%rdx,1)		<-- trapping instruction
>   2e:	0f 85 7e 02 00 00    	jne    0x2b2
>   34:	49 8d bc 24 88 0a 00 	lea    0xa88(%r12),%rdi
>   3b:	00 
>   3c:	4c                   	rex.WR
>   3d:	8b                   	.byte 0x8b
>   3e:	a8 88                	test   $0x88,%al
> 
> Code starting with the faulting instruction
> ===========================================
>    0:	80 3c 11 00          	cmpb   $0x0,(%rcx,%rdx,1)
>    4:	0f 85 7e 02 00 00    	jne    0x288
>    a:	49 8d bc 24 88 0a 00 	lea    0xa88(%r12),%rdi
>   11:	00 
>   12:	4c                   	rex.WR
>   13:	8b                   	.byte 0x8b
>   14:	a8 88                	test   $0x88,%al
> [  356.312254][ T3147] RSP: 0018:ffffc9000883fd20 EFLAGS: 00010002
> [  356.312556][ T3147] RAX: 0000000000000000 RBX: ffff88816ee4c700 RCX: 0000000000000151
> [  356.312933][ T3147] RDX: dffffc0000000000 RSI: 0000000000000000 RDI: 0000000000000a88
> [  356.313310][ T3147] RBP: ffffc9000883fd48 R08: 0000000000000000 R09: 0000000000000000
> [  356.313687][ T3147] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88814f0c8000
> [  356.314063][ T3147] R13: ffff88814f92b700 R14: ffff888161e71450 R15: ffff888161e71408
> [  356.314440][ T3147] FS:  00007f3c72136740(0000) GS:ffff8883af000000(0000) knlGS:0000000000000000
> [  356.314879][ T3147] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  356.315194][ T3147] CR2: 00007f3c708bd000 CR3: 0000000165606000 CR4: 00000000000406f0
> [  356.315573][ T3147] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  356.315950][ T3147] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [  356.316334][ T3147] Call Trace:
> [  356.316498][ T3147]  <TASK>
> [ 356.316645][ T3147] ? show_regs (kbuild/src/consumer/arch/x86/kernel/dumpstack.c:479) 
> [ 356.316859][ T3147] ? die_addr (kbuild/src/consumer/arch/x86/kernel/dumpstack.c:421 kbuild/src/consumer/arch/x86/kernel/dumpstack.c:460) 
> [ 356.317066][ T3147] ? exc_general_protection (kbuild/src/consumer/arch/x86/kernel/traps.c:751 kbuild/src/consumer/arch/x86/kernel/traps.c:693) 
> [ 356.317349][ T3147] ? asm_exc_general_protection (kbuild/src/consumer/arch/x86/include/asm/idtentry.h:574) 
> 
> 
> The kernel config and materials to reproduce are available at:
> https://download.01.org/0day-ci/archive/20250326/202503261534.22d970e8-lkp@intel.com
> 
> 
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
> 
> 

Re: [linux-next:master] [landlock] 9d65581539: WARNING:suspicious_RCU_usage

[Mickaël Salaün]

On Fri, Mar 28, 2025 at 05:26:32PM +0800, Oliver Sang wrote:
> hi, Mickaël Salaün,
> 
> On Fri, Mar 28, 2025 at 09:31:15AM +0100, Mickaël Salaün wrote:
> > On Fri, Mar 28, 2025 at 02:05:59PM +0800, Oliver Sang wrote:
> > > hi, Mickaël Salaün,
> > > 
> > > On Fri, Mar 28, 2025 at 11:00:37AM +0800, Oliver Sang wrote:
> > > > hi, Mickaël Salaün,
> > > > 
> > > > On Thu, Mar 27, 2025 at 07:41:08PM +0100, Mickaël Salaün wrote:
> > > > > Hi Olivier,
> > > > > 
> > > > > I pushed an updated yesterday in linux-next that should fix this issue
> > > > > and this other issue too:
> > > > > https://lore.kernel.org/all/20250326.yee0ba6Yai3m@digikod.net/
> > > > > 
> > > > > Could you please confirm that these issues are really fixed? Or
> > > > > otherwise, please let me know when I should expect (or not) an email
> > > > > from kernel test robot. :)
> > > > 
> > > > ok, we've started the tests for both issues. upon the commit:
> > > > db8da9da41bce (tag: next-20250327, linux-next/master) Add linux-next specific files for 20250327
> > > 
> > > sorry that due to unknown reason, we cannot build sucessfully upon db8da9da41bce
> > > for both tests (which are using randconfig). could you give us the commit-id
> > > of your fix? we could try test upon that fix commit again.
> > 
> > The new commit is 18eb75f3af40 ("landlock: Always allow signals between
> > threads of the same process").
> 
> it turned out the build failure is due to my typo. shamed...
> 
> we finished tests still upon db8da9da41bce, for the WARNING:suspicious_RCU_usage
> issue in this report, we run the tests 20 times, cannot reproduce now.
> 
> for the random issues we reported in
> https://lore.kernel.org/all/202503261534.22d970e8-lkp@intel.com/
> now we cannot reproduce it with db8da9da41bce by 500 runs.

Yes, that makes sense with my fix.

> 
> we think both issues are solved.
> 
> and since db8da9da41bce includes the 18eb75f3af40, we won't test again upon
> 18eb75f3af40. thanks!

Thanks for the confirmation!

> 
> 
> > 
> > > 
> > > > 
> > > > if this is not the correct commit to check, please let us know. thanks!
> > > > 
> > > > > 
> > > > > Regards,
> > > > >  Mickaël
> > > > > 
> > > > > On Wed, Mar 26, 2025 at 04:00:12PM +0800, kernel test robot wrote:
> > > > > > 
> > > > > > hi, Mickaël Salaün,
> > > > > > 
> > > > > > we just reported a random
> > > > > > "Oops:general_protection_fault,probably_for_non-canonical_address#:#[##]SMP_KASAN"
> > > > > > issue in
> > > > > > https://lore.kernel.org/all/202503261534.22d970e8-lkp@intel.com/
> > > > > > 
> > > > > > now we noticed this commit is also in linux-next/master.
> > > > > > 
> > > > > > we don't have enough knowledge to check the difference, but we found a
> > > > > > persistent issue for this commit.
> > > > > > 
> > > > > > 6d9ac5e4d70eba3e 9d65581539252fdb1666917a095
> > > > > > ---------------- ---------------------------
> > > > > >        fail:runs  %reproduction    fail:runs
> > > > > >            |             |             |
> > > > > >            :6          100%           6:6     dmesg.WARNING:suspicious_RCU_usage
> > > > > >            :6          100%           6:6     dmesg.boot_failures
> > > > > >            :6          100%           6:6     dmesg.kernel/pid.c:#suspicious_rcu_dereference_check()usage
> > > > > > 
> > > > > > below full report FYI.
> > > > > > 
> > > > > > 
> > > > > > Hello,
> > > > > > 
> > > > > > kernel test robot noticed "WARNING:suspicious_RCU_usage" on:
> > > > > > 
> > > > > > commit: 9d65581539252fdb1666917a09549c13090fe9e5 ("landlock: Always allow signals between threads of the same process")
> > > > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master
> > > > > > 
> > > > > > [test failed on linux-next/master eb4bc4b07f66f01618d9cb1aa4eaef59b1188415]
> > > > > > 
> > > > > > in testcase: trinity
> > > > > > version: trinity-x86_64-ba2360ed-1_20241228
> > > > > > with following parameters:
> > > > > > 
> > > > > > 	runtime: 300s
> > > > > > 	group: group-00
> > > > > > 	nr_groups: 5
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > config: x86_64-randconfig-101-20250325
> > > > > > compiler: gcc-12
> > > > > > test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
> > > > > > 
> > > > > > (please refer to attached dmesg/kmsg for entire log/backtrace)
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > > > > the same patch/commit), kindly add following tags
> > > > > > | Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > > | Closes: https://lore.kernel.org/oe-lkp/202503261510.f9652c11-lkp@intel.com
> > > > > > 
> > > > > > 
> > > > > > [  166.893101][ T3747] WARNING: suspicious RCU usage
> > > > > > [  166.893462][ T3747] 6.14.0-rc5-00006-g9d6558153925 #1 Not tainted
> > > > > > [  166.893895][ T3747] -----------------------------
> > > > > > [  166.894239][ T3747] kernel/pid.c:414 suspicious rcu_dereference_check() usage!
> > > > > > [  166.894747][ T3747]
> > > > > > [  166.894747][ T3747] other info that might help us debug this:
> > > > > > [  166.894747][ T3747]
> > > > > > [  166.895450][ T3747]
> > > > > > [  166.895450][ T3747] rcu_scheduler_active = 2, debug_locks = 1
> > > > > > [  166.896030][ T3747] 3 locks held by trinity-c2/3747:
> > > > > > [ 166.896415][ T3747] #0: ffff888114a5a930 (&group->mark_mutex){+.+.}-{4:4}, at: fcntl_dirnotify (include/linux/sched/mm.h:332 include/linux/sched/mm.h:386 include/linux/fsnotify_backend.h:279 fs/notify/dnotify/dnotify.c:329) 
> > > > > > [ 166.897165][ T3747] #1: ffff888148bbea60 (&mark->lock){+.+.}-{3:3}, at: fcntl_dirnotify (fs/notify/dnotify/dnotify.c:349) 
> > > > > > [ 166.897831][ T3747] #2: ffff888108a53220 (&f_owner->lock){....}-{3:3}, at: __f_setown (fs/fcntl.c:137) 
> > > > > > [  166.898481][ T3747]
> > > > > > [  166.898481][ T3747] stack backtrace:
> > > > > > [  166.898901][ T3747] CPU: 0 UID: 65534 PID: 3747 Comm: trinity-c2 Not tainted 6.14.0-rc5-00006-g9d6558153925 #1
> > > > > > [  166.898908][ T3747] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> > > > > > [  166.898912][ T3747] Call Trace:
> > > > > > [  166.898916][ T3747]  <TASK>
> > > > > > [ 166.898921][ T3747] dump_stack_lvl (lib/dump_stack.c:123) 
> > > > > > [ 166.898932][ T3747] lockdep_rcu_suspicious (kernel/locking/lockdep.c:6848) 
> > > > > > [ 166.898945][ T3747] pid_task (kernel/pid.c:414 (discriminator 11)) 
> > > > > > [ 166.898954][ T3747] hook_file_set_fowner (security/landlock/fs.c:1651 (discriminator 9)) 
> > > > > > [ 166.898963][ T3747] security_file_set_fowner (arch/x86/include/asm/atomic.h:23 (discriminator 4) include/linux/atomic/atomic-arch-fallback.h:457 (discriminator 4) include/linux/jump_label.h:262 (discriminator 4) security/security.c:3062 (discriminator 4)) 
> > > > > > [ 166.898969][ T3747] __f_setown (fs/fcntl.c:145) 
> > > > > > [ 166.898980][ T3747] fcntl_dirnotify (fs/notify/dnotify/dnotify.c:233 fs/notify/dnotify/dnotify.c:371) 
> > > > > > [ 166.898996][ T3747] do_fcntl (fs/fcntl.c:539) 
> > > > > > [ 166.899002][ T3747] ? f_getown (fs/fcntl.c:448) 
> > > > > > [ 166.899007][ T3747] ? check_prev_add (kernel/locking/lockdep.c:3862) 
> > > > > > [ 166.899011][ T3747] ? do_syscall_64 (arch/x86/entry/common.c:102) 
> > > > > > [ 166.899023][ T3747] ? syscall_exit_to_user_mode (include/linux/entry-common.h:361 kernel/entry/common.c:220) 
> > > > > > [ 166.899038][ T3747] __x64_sys_fcntl (fs/fcntl.c:591 fs/fcntl.c:576 fs/fcntl.c:576) 
> > > > > > [ 166.899050][ T3747] do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) 
> > > > > > [ 166.899062][ T3747] ? find_held_lock (kernel/locking/lockdep.c:5341) 
> > > > > > [ 166.899072][ T3747] ? __lock_release+0x10b/0x440 
> > > > > > [ 166.899076][ T3747] ? __task_pid_nr_ns (include/linux/rcupdate.h:347 include/linux/rcupdate.h:880 kernel/pid.c:514) 
> > > > > > [ 166.899082][ T3747] ? reacquire_held_locks (kernel/locking/lockdep.c:5502) 
> > > > > > [ 166.899087][ T3747] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4470) 
> > > > > > [ 166.899093][ T3747] ? do_syscall_64 (arch/x86/entry/common.c:102) 
> > > > > > [ 166.899099][ T3747] ? do_syscall_64 (arch/x86/entry/common.c:102) 
> > > > > > [ 166.899111][ T3747] ? syscall_exit_to_user_mode (include/linux/entry-common.h:361 kernel/entry/common.c:220) 
> > > > > > [ 166.899119][ T3747] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:469 kernel/locking/lockdep.c:4409) 
> > > > > > [ 166.899124][ T3747] ? do_syscall_64 (arch/x86/entry/common.c:102) 
> > > > > > [ 166.899129][ T3747] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4470) 
> > > > > > [ 166.899134][ T3747] ? do_syscall_64 (arch/x86/entry/common.c:102) 
> > > > > > [ 166.899139][ T3747] ? do_int80_emulation (arch/x86/include/asm/atomic.h:23 include/linux/atomic/atomic-arch-fallback.h:457 include/linux/jump_label.h:262 arch/x86/entry/common.c:230) 
> > > > > > [ 166.899149][ T3747] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) 
> > > > > > [  166.899155][ T3747] RIP: 0033:0x7f55ad007719
> > > > > > [ 166.899159][ T3747] Code: 08 89 e8 5b 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b7 06 0d 00 f7 d8 64 89 01 48
> > > > > > All code
> > > > > > ========
> > > > > >    0:	08 89 e8 5b 5d c3    	or     %cl,-0x3ca2a418(%rcx)
> > > > > >    6:	66 2e 0f 1f 84 00 00 	cs nopw 0x0(%rax,%rax,1)
> > > > > >    d:	00 00 00 
> > > > > >   10:	90                   	nop
> > > > > >   11:	48 89 f8             	mov    %rdi,%rax
> > > > > >   14:	48 89 f7             	mov    %rsi,%rdi
> > > > > >   17:	48 89 d6             	mov    %rdx,%rsi
> > > > > >   1a:	48 89 ca             	mov    %rcx,%rdx
> > > > > >   1d:	4d 89 c2             	mov    %r8,%r10
> > > > > >   20:	4d 89 c8             	mov    %r9,%r8
> > > > > >   23:	4c 8b 4c 24 08       	mov    0x8(%rsp),%r9
> > > > > >   28:	0f 05                	syscall
> > > > > >   2a:*	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax		<-- trapping instruction
> > > > > >   30:	73 01                	jae    0x33
> > > > > >   32:	c3                   	ret
> > > > > >   33:	48 8b 0d b7 06 0d 00 	mov    0xd06b7(%rip),%rcx        # 0xd06f1
> > > > > >   3a:	f7 d8                	neg    %eax
> > > > > >   3c:	64 89 01             	mov    %eax,%fs:(%rcx)
> > > > > >   3f:	48                   	rex.W
> > > > > > 
> > > > > > Code starting with the faulting instruction
> > > > > > ===========================================
> > > > > >    0:	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax
> > > > > >    6:	73 01                	jae    0x9
> > > > > >    8:	c3                   	ret
> > > > > >    9:	48 8b 0d b7 06 0d 00 	mov    0xd06b7(%rip),%rcx        # 0xd06c7
> > > > > >   10:	f7 d8                	neg    %eax
> > > > > >   12:	64 89 01             	mov    %eax,%fs:(%rcx)
> > > > > >   15:	48                   	rex.W
> > > > > > [  166.899164][ T3747] RSP: 002b:00007ffff6eefb48 EFLAGS: 00000246 ORIG_RAX: 0000000000000048
> > > > > > [  166.899168][ T3747] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f55ad007719
> > > > > > [  166.899172][ T3747] RDX: 0000000000000027 RSI: 0000000000000402 RDI: 0000000000000043
> > > > > > [  166.899174][ T3747] RBP: 00007f55ab92f058 R08: 0000000099999999 R09: 00000000377dd000
> > > > > > [  166.899177][ T3747] R10: 0000000084848484 R11: 0000000000000246 R12: 0000000000000048
> > > > > > [  166.899180][ T3747] R13: 00007f55acf036c0 R14: 00007f55ab92f058 R15: 00007f55ab92f000
> > > > > > [  166.899203][ T3747]  </TASK>
> > > > > > 
> > > > > > 
> > > > > > The kernel config and materials to reproduce are available at:
> > > > > > https://download.01.org/0day-ci/archive/20250326/202503261510.f9652c11-lkp@intel.com
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -- 
> > > > > > 0-DAY CI Kernel Test Service
> > > > > > https://github.com/intel/lkp-tests/wiki
> > > > > > 
> > > > > > 

[PATCH v2 6/8] selftests/landlock: Split signal_scoping_threads tests

[Mickaël Salaün]

Split signal_scoping_threads tests into signal_scoping_thread_before
and signal_scoping_thread_after.

Use local variables for thread synchronization.  Fix exported function.
Replace some asserts with expects.

Fixes: c8994965013e ("selftests/landlock: Test signal scoping for threads")
Cc: Günther Noack <gnoack@google.com>
Cc: Tahera Fahimi <fahimitahera@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250318161443.279194-7-mic@digikod.net
---

Changes since v1:
- New patch.
---
 .../selftests/landlock/scoped_signal_test.c   | 49 +++++++++++++------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
index 767f117703b7..d313cb626225 100644
--- a/tools/testing/selftests/landlock/scoped_signal_test.c
+++ b/tools/testing/selftests/landlock/scoped_signal_test.c
@@ -249,47 +249,66 @@ TEST_F(scoped_domains, check_access_signal)
 		_metadata->exit_code = KSFT_FAIL;
 }
 
-static int thread_pipe[2];
-
 enum thread_return {
 	THREAD_INVALID = 0,
 	THREAD_SUCCESS = 1,
 	THREAD_ERROR = 2,
 };
 
-void *thread_func(void *arg)
+static void *thread_sync(void *arg)
 {
+	const int pipe_read = *(int *)arg;
 	char buf;
 
-	if (read(thread_pipe[0], &buf, 1) != 1)
+	if (read(pipe_read, &buf, 1) != 1)
 		return (void *)THREAD_ERROR;
 
 	return (void *)THREAD_SUCCESS;
 }
 
-TEST(signal_scoping_threads)
+TEST(signal_scoping_thread_before)
 {
-	pthread_t no_sandbox_thread, scoped_thread;
+	pthread_t no_sandbox_thread;
 	enum thread_return ret = THREAD_INVALID;
+	int thread_pipe[2];
 
 	drop_caps(_metadata);
 	ASSERT_EQ(0, pipe2(thread_pipe, O_CLOEXEC));
 
-	ASSERT_EQ(0,
-		  pthread_create(&no_sandbox_thread, NULL, thread_func, NULL));
+	ASSERT_EQ(0, pthread_create(&no_sandbox_thread, NULL, thread_sync,
+				    &thread_pipe[0]));
 
-	/* Restricts the domain after creating the first thread. */
+	/* Enforces restriction after creating the thread. */
 	create_scoped_domain(_metadata, LANDLOCK_SCOPE_SIGNAL);
 
-	ASSERT_EQ(0, pthread_kill(no_sandbox_thread, 0));
-	ASSERT_EQ(1, write(thread_pipe[1], ".", 1));
-
-	ASSERT_EQ(0, pthread_create(&scoped_thread, NULL, thread_func, NULL));
-	ASSERT_EQ(0, pthread_kill(scoped_thread, 0));
-	ASSERT_EQ(1, write(thread_pipe[1], ".", 1));
+	EXPECT_EQ(0, pthread_kill(no_sandbox_thread, 0));
+	EXPECT_EQ(1, write(thread_pipe[1], ".", 1));
 
 	EXPECT_EQ(0, pthread_join(no_sandbox_thread, (void **)&ret));
 	EXPECT_EQ(THREAD_SUCCESS, ret);
+
+	EXPECT_EQ(0, close(thread_pipe[0]));
+	EXPECT_EQ(0, close(thread_pipe[1]));
+}
+
+TEST(signal_scoping_thread_after)
+{
+	pthread_t scoped_thread;
+	enum thread_return ret = THREAD_INVALID;
+	int thread_pipe[2];
+
+	drop_caps(_metadata);
+	ASSERT_EQ(0, pipe2(thread_pipe, O_CLOEXEC));
+
+	/* Enforces restriction before creating the thread. */
+	create_scoped_domain(_metadata, LANDLOCK_SCOPE_SIGNAL);
+
+	ASSERT_EQ(0, pthread_create(&scoped_thread, NULL, thread_sync,
+				    &thread_pipe[0]));
+
+	EXPECT_EQ(0, pthread_kill(scoped_thread, 0));
+	EXPECT_EQ(1, write(thread_pipe[1], ".", 1));
+
 	EXPECT_EQ(0, pthread_join(scoped_thread, (void **)&ret));
 	EXPECT_EQ(THREAD_SUCCESS, ret);
 
-- 
2.48.1

[PATCH v2 7/8] selftests/landlock: Add a new test for setuid()

[Mickaël Salaün]

The new signal_scoping_thread_setuid tests check that the libc's
setuid() function works as expected even when a thread is sandboxed with
scoped signal restrictions.

Before the signal scoping fix, this test would have failed with the
setuid() call:

  [pid    65] getpid()                    = 65
  [pid    65] tgkill(65, 66, SIGRT_1)     = -1 EPERM (Operation not permitted)
  [pid    65] futex(0x40a66cdc, FUTEX_WAKE_PRIVATE, 1) = 0
  [pid    65] setuid(1001)                = 0

After the fix, tgkill(2) is successfully leveraged to synchronize
credentials update across threads:

  [pid    65] getpid()                    = 65
  [pid    65] tgkill(65, 66, SIGRT_1)     = 0
  [pid    66] <... read resumed>0x40a65eb7, 1) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
  [pid    66] --- SIGRT_1 {si_signo=SIGRT_1, si_code=SI_TKILL, si_pid=65, si_uid=1000} ---
  [pid    66] getpid()                    = 65
  [pid    66] setuid(1001)                = 0
  [pid    66] futex(0x40a66cdc, FUTEX_WAKE_PRIVATE, 1) = 0
  [pid    66] rt_sigreturn({mask=[]})     = 0
  [pid    66] read(3,  <unfinished ...>
  [pid    65] setuid(1001)                = 0

Test coverage for security/landlock is 92.9% of 1053 lines according to
gcc/gcov-14.

Fixes: c8994965013e ("selftests/landlock: Test signal scoping for threads")
Cc: Günther Noack <gnoack@google.com>
Cc: Tahera Fahimi <fahimitahera@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250318161443.279194-8-mic@digikod.net
---

Changes since v1:
- New patch.
---
 tools/testing/selftests/landlock/common.h     |  1 +
 .../selftests/landlock/scoped_signal_test.c   | 59 +++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index 6064c9ac0532..076a9a625c98 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -41,6 +41,7 @@ static void _init_caps(struct __test_metadata *const _metadata, bool drop_all)
 		CAP_MKNOD,
 		CAP_NET_ADMIN,
 		CAP_NET_BIND_SERVICE,
+		CAP_SETUID,
 		CAP_SYS_ADMIN,
 		CAP_SYS_CHROOT,
 		/* clang-format on */
diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
index d313cb626225..d8bf33417619 100644
--- a/tools/testing/selftests/landlock/scoped_signal_test.c
+++ b/tools/testing/selftests/landlock/scoped_signal_test.c
@@ -253,6 +253,7 @@ enum thread_return {
 	THREAD_INVALID = 0,
 	THREAD_SUCCESS = 1,
 	THREAD_ERROR = 2,
+	THREAD_TEST_FAILED = 3,
 };
 
 static void *thread_sync(void *arg)
@@ -316,6 +317,64 @@ TEST(signal_scoping_thread_after)
 	EXPECT_EQ(0, close(thread_pipe[1]));
 }
 
+struct thread_setuid_args {
+	int pipe_read, new_uid;
+};
+
+void *thread_setuid(void *ptr)
+{
+	const struct thread_setuid_args *arg = ptr;
+	char buf;
+
+	if (read(arg->pipe_read, &buf, 1) != 1)
+		return (void *)THREAD_ERROR;
+
+	/* libc's setuid() should update all thread's credentials. */
+	if (getuid() != arg->new_uid)
+		return (void *)THREAD_TEST_FAILED;
+
+	return (void *)THREAD_SUCCESS;
+}
+
+TEST(signal_scoping_thread_setuid)
+{
+	struct thread_setuid_args arg;
+	pthread_t no_sandbox_thread;
+	enum thread_return ret = THREAD_INVALID;
+	int pipe_parent[2];
+	int prev_uid;
+
+	disable_caps(_metadata);
+
+	/* This test does not need to be run as root. */
+	prev_uid = getuid();
+	arg.new_uid = prev_uid + 1;
+	EXPECT_LT(0, arg.new_uid);
+
+	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+	arg.pipe_read = pipe_parent[0];
+
+	/* Capabilities must be set before creating a new thread. */
+	set_cap(_metadata, CAP_SETUID);
+	ASSERT_EQ(0, pthread_create(&no_sandbox_thread, NULL, thread_setuid,
+				    &arg));
+
+	/* Enforces restriction after creating the thread. */
+	create_scoped_domain(_metadata, LANDLOCK_SCOPE_SIGNAL);
+
+	EXPECT_NE(arg.new_uid, getuid());
+	EXPECT_EQ(0, setuid(arg.new_uid));
+	EXPECT_EQ(arg.new_uid, getuid());
+	EXPECT_EQ(1, write(pipe_parent[1], ".", 1));
+
+	EXPECT_EQ(0, pthread_join(no_sandbox_thread, (void **)&ret));
+	EXPECT_EQ(THREAD_SUCCESS, ret);
+
+	clear_cap(_metadata, CAP_SETUID);
+	EXPECT_EQ(0, close(pipe_parent[0]));
+	EXPECT_EQ(0, close(pipe_parent[1]));
+}
+
 const short backlog = 10;
 
 static volatile sig_atomic_t signal_received;
-- 
2.48.1