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

["Mickaël Salaün" <mic@digikod.net>]

On Tue, Apr 07, 2026 at 09:30:40AM +0200, Günther Noack wrote:
> 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.

old is only a partially initialized credential, and the Landlock
part is not set yet, which is the goal of hook_transfer_creds(), so
there is no leak.

> 
> I would suggest to use the helper landlock_cred_copy() from cred.h for

This is not required but if we would like to do it anyway, that would
not be backportable and would introduce a (minimal) performance penalty.

> 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>

I'll keep your tag if this patch is ok with you as-is.

> 
> –Günther
>