Re: [PATCH bpf-next 3/5] selinux: use kernel sid in security_bpf_*

[bot+bpf-ci]

> The security_bpf hooks provides a boolean to indicate if the call is
> coming from within the kernel or not. If true, use the kernel SID
> instead of relying on the current process SID.
>
> For the token-aware functions, the kernel sid is used to decide on the
> access, but the caller remains owner of the object (program or map).
>
> Signed-off-by: Thiébaud Weksteen <tweek@xxxxxxxxxx>

The changelog claims that for the token-aware hooks "the kernel sid is
used to decide on the access". Is that accurate when a token is present?

Looking at selinux_bpf_map_create():

    if (!token)
            ssid = kernel ? SECINITSID_KERNEL : bpfsec->sid;
    else
            ssid = selinux_bpffs_creator_sid(attr->map_token_fd);

The same pattern appears in selinux_bpf_prog_load(). When a token is
supplied, the kernel argument has no effect - the SID comes from
selinux_bpffs_creator_sid() regardless. Should the commit message clarify
that the kernel SID applies only when no token is present?

This looks like a bug fix for the kernel parameter support added in
082f1db02c80. Should this include:

  Fixes: 082f1db02c80 ("security: Propagate caller information in bpf hooks")

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f197cf476190d..e7c5993f69545 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -7181,7 +7181,7 @@ static int selinux_ib_alloc_security(void *ib_sec)
>  static int selinux_bpf(int cmd, union bpf_attr *attr,
>  		       unsigned int size, bool kernel)
>  {
> -	u32 sid = current_sid();
> +	u32 sid = kernel ? SECINITSID_KERNEL : current_sid();
>  	int ret;
>
>  	if (selinux_policycap_bpf_token_perms())
> @@ -7296,7 +7296,7 @@ static int selinux_bpf_map_create(struct bpf_map *map, union bpf_attr *attr,
>  	bpfsec->sid = current_sid();
>
>  	if (!token)
> -		ssid = bpfsec->sid;
> +		ssid = kernel ? SECINITSID_KERNEL : bpfsec->sid;
>  	else
>  		ssid = selinux_bpffs_creator_sid(attr->map_token_fd);

The kernel argument of the security_bpf hooks is not a "trusted kernel
origin" indicator - it is uattr.is_kernel, which only indicates whether
the bpf_attr buffer lives in kernel or user address space. Can
substituting SECINITSID_KERNEL based on that flag change the SELinux
decision for a pre-existing userspace code path?

This path is reachable without BPF_LOADER_LOAD_FD. Looking at
kernel/bpf/syscall.c, syscall_prog_func_proto() exposes bpf_sys_bpf() to
any BPF_PROG_TYPE_SYSCALL program that passes bpf_token_capable(token,
CAP_PERFMON). BPF_CALL_3(bpf_sys_bpf, ...) accepts BPF_MAP_CREATE and
BPF_PROG_LOAD and calls:

    return __sys_bpf(cmd, KERNEL_BPFPTR(attr), attr_size,
                     KERNEL_BPFPTR(NULL), 0);

which triggers:

    security_bpf(cmd, &attr, size, uattr.is_kernel);              /* true */
    security_bpf_map_create(map, attr, token, uattr.is_kernel);   /* true */
    security_bpf_prog_load(prog, attr, token, uattr.is_kernel);   /* true */

Such a program is loaded and executed entirely at userspace request via
BPF_PROG_TEST_RUN. This is the libbpf light-skeleton loader
(tools/lib/bpf/skel_internal.h declares kern_sys_bpf(), used by
bpf_load_and_run()), which predates this series.

For a caller in domain X using a light skeleton:

Before this patch:
  selinux_bpf()            checks avc_has_perm(X, X, BPF, map_create)
  selinux_bpf_map_create() checks avc_has_perm(X, X, BPF, map_create)

After this patch:
  selinux_bpf()            checks avc_has_perm(kernel_t, kernel_t, BPF,
                                                map_create)
  selinux_bpf_map_create() checks avc_has_perm(kernel_t, X, BPF,
                                                map_create)
  selinux_bpf_prog_load()  checks avc_has_perm(kernel_t, X, BPF,
                                                prog_load)

The rule that used to authorize this (allow X self:bpf map_create) no
longer applies, so existing policies get new AVC denials on a path that
worked before. Conversely, in policies where kernel_t is broadly
privileged, domain X gains map_create and prog_load permissions it was
never granted.

This behavior change is not gated. The immediately preceding behavior
change in the same function is gated on selinux_policycap_bpf_token_perms(),
which is the established SELinux convention for this kind of decision
change. Should this new SID substitution also be gated on a policy
capability?

> @@ -7314,7 +7314,7 @@ static int selinux_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr,
>  	bpfsec->sid = current_sid();
>
>  	if (!token)
> -		ssid = bpfsec->sid;
> +		ssid = kernel ? SECINITSID_KERNEL : bpfsec->sid;
>  	else
>  		ssid = selinux_bpffs_creator_sid(attr->prog_token_fd);


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