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

["Günther Noack" <gnoack3000@gmail.com>]

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