Re: [PATCH bpf-next 08/13] bpf: Add the bpf_landlock_put_ruleset kfunc and ruleset destructor

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf_landlock_put_ruleset is incorrectly restricted to only two LSM hooks, contradicting the documented design.
- [Medium] bpf_landlock_put_ruleset is marked KF_SLEEPABLE, which breaks kptr semantics by preventing non-sleepable programs from releasing acquired rulesets.
--

--- Patch [8]: [PATCH bpf-next 08/13] bpf: Add the bpf_landlock_put_ruleset kfunc and ruleset destructor ---
commit c8093884a93ea01a073e8cc7633e2b790c416510
Author: Justin Suess <[email protected]>

bpf: Add the bpf_landlock_put_ruleset kfunc and ruleset destructor

This commit adds a release kfunc for Landlock ruleset references. It provides
a way to drop references when handing them over through a map kptr field,
registering a destructor for map teardown.

> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index dd58c5bd01193..877dd03526077 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c

[ ... ]

>  BTF_KFUNCS_START(bpf_landlock_kfunc_ids)
> +BTF_ID_FLAGS(func, bpf_landlock_put_ruleset, KF_RELEASE | KF_SLEEPABLE)
>  BTF_KFUNCS_END(bpf_landlock_kfunc_ids)

[Severity: Medium]
The commit message mentions that "the release path may thus run from a
context that cannot sleep". Does adding the KF_SLEEPABLE flag here contradict
that requirement?

It appears this flag will cause the verifier to reject non-sleepable LSM
programs (like standard RCU-based programs) if they try to release a ruleset
reference they just acquired from a map.

[ ... ]

> @@ -498,6 +540,17 @@ static int bpf_landlock_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)
>  	case BPF_PROG_TYPE_SYSCALL:
>  		return 0;
>  	case BPF_PROG_TYPE_LSM:
> +		/*
> +		 * BPF_LSM_CGROUP programs run under classic RCU and
> +		 * cannot sleep.
> +		 */
> +		if (prog->expected_attach_type == BPF_LSM_CGROUP)
> +			return -EACCES;
> +
> +		if (!btf_id_set_contains(&bpf_landlock_kfunc_hooks,
> +					 prog->aux->attach_btf_id))
> +			return -EACCES;
> +
>  		return 0;

[Severity: Medium]
Does this check unintentionally restrict the bpf_landlock_put_ruleset kfunc
from being used by other LSM hooks?

Because bpf_landlock_put_ruleset is evaluated by this filter, it seems any
LSM program attached to a hook outside of bpf_landlock_kfunc_hooks
(bprm_creds_for_exec and bprm_creds_from_file) will fail verification if it
tries to release a ruleset. If an LSM program on a different hook retrieves a
ruleset via bpf_kptr_xchg, shouldn't it be permitted to release it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8