Re: [PATCH bpf-next v3 11/15] landlock: Factor the credential restriction out of landlock_restrict_self()

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 5989 bytes --]

> diff --git a/security/landlock/cred.c b/security/landlock/cred.c
> index 03449c26247e..f02706f12c7d 100644
> --- a/security/landlock/cred.c
> +++ b/security/landlock/cred.c
> @@ -8,14 +8,146 @@
>   */
>
>  #include <linux/binfmts.h>
> +#include <linux/bits.h>
>  #include <linux/cred.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>

This isn't a bug, but is <linux/errno.h> needed here?  The new code
returns PTR_ERR(new_dom) (from <linux/err.h>) and never names an -Exxx
constant.

> +#include <linux/lsm_hooks.h>
> +#include <linux/mutex.h>
> +#include <uapi/linux/landlock.h>
>
>  #include "common.h"
>  #include "cred.h"
> +#include "domain.h"
>  #include "ruleset.h"
>  #include "setup.h"
>
> +#include <trace/events/landlock.h>
> +
> +/**
> + * landlock_prepare_restriction - Compute a credential restriction
> + *
> + * @llcred: Landlock credentials to restrict: provides the parent domain and
> + *          the previous log configuration.  Not modified.
> + * @ruleset: Ruleset to enforce, or NULL for a log-configuration-only change.
> + * @flags: landlock_restrict_self(2) flags.  The caller is responsible for
> + *         validating them against the set of flags it supports.
> + * @restriction: Computed restriction.  On success, holds a reference on
> + *               @restriction->domain (if any), which
> + *               landlock_apply_restriction() transfers to the restricted
> + *               credentials.
> + *
> + * The restriction builds on @llcred's current state: the caller must apply
> + * it to (or stage it for) these same credentials.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +int landlock_prepare_restriction(
> +	const struct landlock_cred_security *const llcred,
> +	struct landlock_ruleset *const ruleset, const u32 flags,
> +	struct landlock_restriction *const restriction)
> +{

[ ... ]

> +#ifdef CONFIG_SECURITY_LANDLOCK_LOG
> +	new_dom->hierarchy->log_same_exec = log_same_exec;
> +	new_dom->hierarchy->log_new_exec = log_new_exec;
> +	/*
> +	 * The creation event fired above, so move the domain out of
> +	 * LANDLOCK_LOG_UNCOMMITTED: its free_domain event must fire
> +	 * too, even if the domain is dropped before being enforced.
> +	 * Audit logging may still be disabled (DISABLED); tracing
> +	 * observes it anyway.
> +	 */
> +	if ((!log_same_exec && !log_new_exec) || !prev_log_subdomains)
> +		new_dom->hierarchy->log_status = LANDLOCK_LOG_DISABLED;
> +	else
> +		new_dom->hierarchy->log_status = LANDLOCK_LOG_PENDING;
> +#endif /* CONFIG_SECURITY_LANDLOCK_LOG */

The LANDLOCK_LOG_UNCOMMITTED -> PENDING/DISABLED transition moves from
sys_landlock_restrict_self() into landlock_prepare_restriction(), but
comments that document this contract by naming the old site remain
unrevisioned, and no later commit in the 02756181d382..1cc3305917e4
range updates them either.

In security/landlock/domain.c (landlock_init_hierarchy_log):

     /*
      * The hierarchy is born unobservable: landlock_restrict_self() moves it
      * out of LANDLOCK_LOG_UNCOMMITTED once it has emitted the creation
      * event, so the matching free_domain event fires for it and not for a
      * hierarchy whose creation was never observed.
      */

In security/landlock/trace.c (landlock_trace_free_domain):

      * Fires only for a hierarchy whose creation event was emitted, i.e. one that
      * left LANDLOCK_LOG_UNCOMMITTED in landlock_restrict_self().  ...
      * while a domain that landlock_restrict_self() created and a
      * thread-sync failure then aborted still fires free_domain, ...

In include/trace/events/landlock.h (Lifecycle consistency block):

The window is still described as 'A rare thread-sync failure aborts the
just-created domain' even though the new helper's documentation
(cred.c:89-93) states it also covers an aborted execution, introduced
two commits later in 0a88c757f4f0.

After this commit, landlock_prepare_restriction() performs the
transition, and 0a88c757f4f0 adds a second, non-syscall caller
(bprm_apply_policy_object()), so 'landlock_restrict_self()' no longer
enforces this invariant nor provides the only path that can create and
drop a domain.

Behaviour is unaffected, but should these references be updated to
reflect the new code structure?

> +
> +	restriction->domain = new_dom;
> +	return 0;
> +}

[ ... ]

> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index cb294a3582ae..9451376ccf50 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -9,7 +9,6 @@
>
>  #include <asm/current.h>
>  #include <linux/anon_inodes.h>
> -#include <linux/bitops.h>
>  #include <linux/build_bug.h>
>  #include <linux/capability.h>
>  #include <linux/cleanup.h>

[ ... ]

> @@ -546,10 +544,9 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
>  	struct landlock_ruleset *ruleset __free(landlock_put_ruleset) = NULL;
>  	struct landlock_domain *new_dom = NULL;
>  	struct cred *new_cred;

This isn't a bug, but is the `= NULL` initializer still needed here?
Before the refactor, new_dom was only assigned inside the if (ruleset)
block, so the initializer was load-bearing for the later if (ruleset)
trace_landlock_enforce_domain(new_dom, ...) call. After the split,
new_dom is assigned unconditionally from restriction.domain, and every
path between the declaration and that assignment is a plain return.  It
is the only `= NULL` pointer initializer left in the file apart from the
__free() declaration on the line above.

> -	struct landlock_cred_security *new_llcred;
> +	struct landlock_restriction restriction;
>  	bool process_wide;
> -	bool __maybe_unused log_same_exec, log_new_exec, log_subdomains,
> -		prev_log_subdomains;
> +	int err;

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34398515657