Re: [PATCH bpf-next 01/11] bpf: Add a bpf keyring for program signature validation

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] `VERIFY_USE_BPF_KEYRING` aliases a valid user keyring ID, making user keyring ID 3 unusable for BPF signature verification.
--

--- Patch [1]: [PATCH bpf-next 01/11] bpf: Add a bpf keyring for program signature validation ---
commit b7fa8763dbdc44199a6b443ce9ddca2d84edc3b7
Author: Daniel Borkmann <[email protected]>

bpf: Add a bpf keyring for program signature validation

This commit adds a dedicated bpf keyring for program signature validation,
selectable via a new well-known keyring_id VERIFY_USE_BPF_KEYRING. The
keyring is sealed empty at init but can be left open for provisioning
via a boot parameter.

> diff --git a/include/linux/verification.h b/include/linux/verification.h
> index dec7f2beabfd4..1cb59ddda2509 100644
> --- a/include/linux/verification.h
> +++ b/include/linux/verification.h
> @@ -18,6 +18,16 @@
>  #define VERIFY_USE_SECONDARY_KEYRING ((struct key *)1UL)
>  #define VERIFY_USE_PLATFORM_KEYRING  ((struct key *)2UL)
>  
> +/*
> + * The id of BPF's ".bpf" keyring, reserved from the same space. It is
> + * explicitly not a sentinel like the two above as BPF resolves it to
> + * the keyring itself and passes that, so verify_pkcs7_signature() never
> + * sees this value, and system_keyring_id_check() must keep rejecting it.
> + * Left as a plain integer so that handing it over as @trusted_keys does
> + * not compile.
> + */
> +#define VERIFY_USE_BPF_KEYRING	3

[Severity: Low]
Does this constant overlap with the valid user keyring ID space?

Since user keyring IDs generated by key_alloc_serial() are positive integers
that can start from 3 (avoiding 0, 1, and 2), 3 is technically a valid,
assignable user keyring ID.

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 16de43f172382..8075bdbee6254 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20908,10 +20910,17 @@ static int bpf_prog_verify_signature(struct bpf_verifier_env *env,
>  	if (!attr->signature_size ||
>  	    attr->signature_size > KMALLOC_MAX_CACHE_SIZE)
>  		return -EINVAL;
> -	if (system_keyring_id_check(attr->keyring_id) == 0)
> +	if (attr->keyring_id == VERIFY_USE_BPF_KEYRING) {
> +		key = bpf_lookup_keyring();

[Severity: Low]
If a user happens to generate a keyring with ID 3 and attempts to use it for
BPF signature verification here in bpf_prog_verify_signature(), wouldn't
this code incorrectly resolve it to the system .bpf keyring instead of their
intended user keyring?

While the probability of receiving serial ID 3 is very small, it appears to
create a structural API design flaw because a sentinel value intercepts a
valid user-space ID.

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

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