Re: [PATCH v1 1/2] landlock: Fix log_subdomains_off inheritance across fork()

Günther Noack
2026-04-07T07:30:49Z

Hello!

On Sat, Apr 04, 2026 at 10:49:57AM +0200, Mickaël Salaün wrote:
> hook_cred_transfer() only copies the Landlock security blob when the
> source credential has a domain. This is inconsistent with
> landlock_restrict_self() which can set log_subdomains_off on a
> credential without creating a domain (via the ruleset_fd=-1 path): the
> field is committed but not preserved across fork() because the child's
> prepare_creds() calls hook_cred_transfer() which skips the copy when
> domain is NULL.
>
> This breaks the documented use case where a process mutes subdomain logs
> before forking sandboxed children: the children lose the muting and
> their domains produce unexpected audit records.
>
> Fix this by unconditionally copying the Landlock credential blob.
> landlock_get_ruleset(NULL) is already a safe no-op.
>
> Cc: Günther Noack <gnoack@google.com>
> Cc: stable@vger.kernel.org
> Fixes: ead9079f7569 ("landlock: Add LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF")
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
> security/landlock/cred.c | 6 +-
> tools/testing/selftests/landlock/audit_test.c | 88 +++++++++++++++++++
> 2 files changed, 90 insertions(+), 4 deletions(-)
>
> diff --git a/security/landlock/cred.c b/security/landlock/cred.c
> index 0cb3edde4d18..cc419de75cd6 100644
> --- a/security/landlock/cred.c
> +++ b/security/landlock/cred.c
> @@ -22,10 +22,8 @@ static void hook_cred_transfer(struct cred *const new,
> const struct landlock_cred_security *const old_llcred =
> landlock_cred(old);
>
> - if (old_llcred->domain) {
> - landlock_get_ruleset(old_llcred->domain);
> - *landlock_cred(new) = *old_llcred;
> - }
> + landlock_get_ruleset(old_llcred->domain);
> + *landlock_cred(new) = *old_llcred;

This fix looks correct for the hook_cred_prepare() case (and of
course, hook_cred_prepare() calls hook_cred_transfer() in Landlock).

But I'm afraid I might have spotted another issue here:

If I look at the code in security/keys/process_keys.c, where
security_tranfer_creds() is called, the "old" object is actually
already initialized, and if we are not checking for that, I think we
are leaking memory.

I would suggest to use the helper landlock_cred_copy() from cred.h for
that. This one is anyway supposed to be the central place for this
copying logic, and it is safe to use with zeroed-out target objects
(because the put is safe for the NULL-pointer).

Maybe this is worth updating while we are at it?

> }
>
> static int hook_cred_prepare(struct cred *const new,
> diff --git a/tools/testing/selftests/landlock/audit_test.c b/tools/testing/selftests/landlock/audit_test.c
> index 46d02d49835a..20099b8667e7 100644
> --- a/tools/testing/selftests/landlock/audit_test.c
> +++ b/tools/testing/selftests/landlock/audit_test.c
> @@ -279,6 +279,94 @@ TEST_F(audit, thread)
> &audit_tv_default, sizeof(audit_tv_default)));
> }
>
> +/*
> + * Verifies that log_subdomains_off set via the ruleset_fd=-1 path (without
> + * creating a domain) is inherited by children across fork(). This exercises
> + * the hook_cred_transfer() fix: the Landlock credential blob must be copied
> + * even when the source credential has no domain.
> + *
> + * Phase 1 (baseline): a child without muting creates a domain and triggers a
> + * denial that IS logged.
> + *
> + * Phase 2 (after muting): the parent mutes subdomain logs, forks another child
> + * who creates a domain and triggers a denial that is NOT logged.
> + */
> +TEST_F(audit, log_subdomains_off_fork)
> +{
> + const struct landlock_ruleset_attr ruleset_attr = {
> + .scoped = LANDLOCK_SCOPE_SIGNAL,
> + };
> + struct audit_records records;
> + int ruleset_fd, status;
> + pid_t child;
> +
> + ruleset_fd =
> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> + ASSERT_LE(0, ruleset_fd);
> +
> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> +
> + /*
> + * Phase 1: forks a child that creates a domain and triggers a denial
> + * before any muting. This proves the audit path works.
> + */
> + child = fork();
> + ASSERT_LE(0, child);
> + if (child == 0) {
> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0));
> + ASSERT_EQ(-1, kill(getppid(), 0));
> + ASSERT_EQ(EPERM, errno);
> + _exit(0);
> + return;
> + }
> +
> + ASSERT_EQ(child, waitpid(child, &status, 0));
> + ASSERT_EQ(true, WIFEXITED(status));
> + ASSERT_EQ(0, WEXITSTATUS(status));
> +
> + /* The denial must be logged (baseline). */
> + EXPECT_EQ(0, matches_log_signal(_metadata, self->audit_fd, getpid(),
> + NULL));
> +
> + /* Drains any remaining records (e.g. domain allocation). */
> + EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
> +
> + /*
> + * Mutes subdomain logs without creating a domain. The parent's
> + * credential has domain=NULL and log_subdomains_off=1.
> + */
> + ASSERT_EQ(0, landlock_restrict_self(
> + -1, LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF));
> +
> + /*
> + * Phase 2: forks a child that creates a domain and triggers a denial.
> + * Because log_subdomains_off was inherited via fork(), the child's
> + * domain has log_status=LANDLOCK_LOG_DISABLED.
> + */
> + child = fork();
> + ASSERT_LE(0, child);
> + if (child == 0) {
> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0));
> + ASSERT_EQ(-1, kill(getppid(), 0));
> + ASSERT_EQ(EPERM, errno);
> + _exit(0);
> + return;
> + }
> +
> + ASSERT_EQ(child, waitpid(child, &status, 0));
> + ASSERT_EQ(true, WIFEXITED(status));
> + ASSERT_EQ(0, WEXITSTATUS(status));
> +
> + /* No denial record should appear. */
> + EXPECT_EQ(-EAGAIN, matches_log_signal(_metadata, self->audit_fd,
> + getpid(), NULL));
> +
> + EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
> + EXPECT_EQ(0, records.access);
> +
> + EXPECT_EQ(0, close(ruleset_fd));
> +}
> +
> FIXTURE(audit_flags)
> {
> struct audit_filter audit_filter;
> --
> 2.53.0
>

Test looks fine.

While I do still think we should investigate the memory leak, this
commit is, as it is, already a strict improvement over what we had
before, so:

Reviewed-by: Günther Noack <gnoack3000@gmail.com>

–Günther