Re: [PATCH bpf-next 05/11] selftests/bpf: Add a test for the sealed bpf keyring

> diff --git a/tools/testing/selftests/bpf/prog_tests/signed_loader.c b/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> index 0c5294738d6ce..94b57e7cdab39 100644
> --- a/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> +++ b/tools/testing/selftests/bpf/prog_tests/signed_loader.c

[ ... ]

> @@ -627,6 +630,28 @@ static void signature_bad_keyring(void)
>  	gen_loader_fixture_fini(&f);
>  }
>
> +static void bpf_keyring_sealed(void)
> +{
> +	static const __u8 junk[64] = {};
> +	struct gen_loader_fixture f;
> +	int fd;
> +
> +	if (gen_loader_fixture_init(&f) == 0) {
> +		/*
> +		 * Without bpf.keyring_unsealed=1 on the command line the bpf
> +		 * keyring is sealed empty during boot, so it is never handed
> +		 * out and a load naming it fails with -ENOKEY before the
> +		 * signature bytes are examined.
> +		 */
> +		fd = load_loader(f.gopts.insns, f.gopts.insns_sz, -1, junk,
> +				 sizeof(junk), BPF_KEYRING_BPF, 0);
> +		ASSERT_EQ(fd, -ENOKEY, "sealed bpf keyring rejected");

A subsystem pattern flags this as potentially concerning: the subtest is
named bpf_keyring_sealed and asserts -ENOKEY, but does -ENOKEY actually
distinguish sealing from the keyring merely being empty?

Looking at bpf_prog_verify_signature() in kernel/bpf/verifier.c, the
-ENOKEY path is taken when:

    if (!key) {
            if (attr->keyring_id == VERIFY_USE_BPF_KEYRING) {
                    verbose(env, "the bpf keyring is empty or has not been restricted\\n");
                    return -ENOKEY;

and key is NULL whenever either condition in bpf_lookup_keyring()
(kernel/bpf/keys.c) holds:

    if (!READ_ONCE(bpf_keyring->keys.nr_leaves_on_tree) ||
        !READ_ONCE(bpf_keyring->restrict_link))
            return NULL;

In the boot state this test exercises (bpf.keyring_unsealed unset), both
conditions are true at once: bpf_keyring_init() calls keyring_restrict() and
the keyring has zero keys. The asserted -ENOKEY is therefore produced by the
emptiness check on its own.

If the sealing under test regressed - say keyring_restrict() were dropped
from bpf_keyring_init(), or it failed and took the pr_err("cannot seal bpf
keyring") path that leaves bpf_keyring NULL - the keyring would still be
empty at boot and the load would still return -ENOKEY. Would the subtest
still pass in that case?

This is confirmed later in the series: baedf3d941d9c ("selftests/bpf: Add
tests for bpf keyring in signed loader") adds bpf_keyring_provisioned(),
which enrolls a key and then asserts -ENOKEY with the message "unrestricted
keyring still not consulted" - a populated-but-unrestricted keyring yields
the same errno this subtest asserts for an empty-and-restricted one.

The subtest does verify the keyring_id routing: without the
VERIFY_USE_BPF_KEYRING special case, keyring_id 3 would return -EINVAL
instead, so -ENOKEY vs -EINVAL is a real distinction. The question is
whether the sealing aspect of the name is fully backed by what the kernel
path actually checks.

Would renaming the subtest and assert message to describe what is checked
(an unprovisioned bpf keyring is never handed out) rather than the sealing
mechanism avoid claiming more than the -ENOKEY assertion verifies, or should
the sealing claim be left to bpf_keyring_provisioned() which separates the
two conditions?

> +		if (fd >= 0)
> +			close(fd);
> +	}
> +	gen_loader_fixture_fini(&f);
> +}

[ ... ]


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