Re: [PATCH bpf-next v3 09/15] selftests/bpf: Add tests for the LSM policy object kfuncs

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_policy_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/lsm_policy_kfuncs.c
> new file mode 100644
> index 000000000000..9f4ffb5f47be
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lsm_policy_kfuncs.c
> @@ -0,0 +1,54 @@

[ ... ]

> +static void test_from_fd_null(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts);
> +	struct lsm_policy_kfuncs *skel;
> +	char tmp_path[] = "/tmp/lsm_policy_kfuncs_XXXXXX";
> +	int tmp_fd, err;
> +
> +	tmp_fd = mkstemp(tmp_path);
> +	if (!ASSERT_GE(tmp_fd, 0, "mkstemp"))
> +		return;
> +
> +	skel = lsm_policy_kfuncs__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
> +		goto out_close;
> +	skel->bss->plain_fd = tmp_fd;
> +
> +	err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.check_from_fd),
> +				     &opts);
> +	if (!ASSERT_OK(err, "check_from_fd_run") ||
> +	    !ASSERT_OK(opts.retval, "check_from_fd_retval"))
> +		goto out_destroy;
> +
> +	ASSERT_TRUE(skel->bss->got_null_for_bad_fd, "bad_fd_null");
> +	ASSERT_TRUE(skel->bss->got_null_for_plain_fd, "plain_fd_null");
> +	ASSERT_TRUE(skel->bss->got_null_for_bad_flags, "bad_flags_null");
                                                   ^^^^^^^^^^^^^^^^
A subsystem pattern flags this as potentially concerning: the
"bad_flags_null" assertion passes whether or not the kernel honors the
reserved-flags contract.

The fd used for the flags test is the plain temporary file created above,
which no LSM claims. In bpf_lsm_policy_from_fd() (security/bpf_lsm_kfuncs.c)
there are two independent paths to NULL for this input:

    if (flags)
            return NULL;                    /* the check under test */

    lsm_for_each_hook(scall, policy_object_from_fd) {
            err = scall->hl->hook.policy_object_from_fd(fd, &object);
            if (err == -EOPNOTSUPP)
                    continue;
            ...
    }
    return NULL;                    /* unrelated: nobody claims a /tmp file */

Because the /tmp file is claimed by no LSM, the loop falls through to the
trailing return NULL on its own. Delete the if (flags) guard entirely and
"bad_flags_null" still passes. The assertion therefore holds whether or not
the kernel implements the reserved-flags contract that the commit message
claims this test verifies.

The other two arms do discriminate once an LSM implements the hook: for
fd == -1 the owning LSM's policy_object_from_fd() fails with a
non-EOPNOTSUPP error, and for a plain fd the LSM must return -EOPNOTSUPP
rather than claiming the file. Only the flags arm is unconditioned.

A discriminating flags test needs an fd that would resolve successfully
with flags == 0, which by design belongs in the LSM-specific test rather
than here. Could the commit message and comment clarify that the
reserved-flags contract is only partially covered here, or could this test
be deferred to an LSM that provides real policy objects?

> diff --git a/tools/testing/selftests/bpf/progs/lsm_policy_kfuncs_failure.c b/tools/testing/selftests/bpf/progs/lsm_policy_kfuncs_failure.c
> new file mode 100644
> index 000000000000..04080838aefd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/lsm_policy_kfuncs_failure.c
> @@ -0,0 +1,154 @@

[ ... ]

> +/*
> + * The fd kfunc is exclusive to syscall programs: it must be rejected
> + * in an LSM program, even on an allowed hook.
> + */
> +SEC("lsm.s/bprm_creds_for_exec")
> +__failure __msg("calling kernel function bpf_lsm_policy_from_fd is not allowed")
> +int BPF_PROG(lsm_get, struct linux_binprm *bprm)
          ^^^^^^^^

[ ... ]

> +/*
> + * The enforcement kfunc is exclusive to the sleepable bprm LSM
> + * hooks: it must be rejected in a syscall program.
> + */
> +SEC("syscall")
> +__failure __msg("calling kernel function bpf_lsm_policy_apply_bprm is not allowed")
> +int syscall_restrict(void *ctx)
     ^^^^^^^^^^^^^^^

This isn't a bug, but six of the eight programs in this file are named
after the condition they pin down (tracing_prog, wrong_hook,
nonsleepable_prog, leak_policy, acquire_untrusted, leak_shared_policy),
which makes a RUN_TESTS failure line self-explanatory. The other two are
named after the program type plus an operation that doesn't appear in the
interface: lsm_get and syscall_restrict. There is no 'get' or 'restrict'
kfunc. Would names in the same style as the neighbours - e.g. lsm_from_fd
and syscall_apply_bprm - make the failing subtest easier to place from
the RUN_TESTS output?


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