Re: [PATCH bpf-next v4 01/16] bpf: Check exclusive maps once the program digest is final

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: __add_used_map() claims exclusive maps before check_excl_maps() runs
- [High] bpf: unauthenticated CO-RE relocations on signed progs in bpf_check()
--

commit 8772d7da5dbe0ad0dc32abd5edb328e0e62a1dbe
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Check exclusive maps once the program digest is final

This commit moves the exclusive map hash comparison out of
check_map_prog_compatibility() into check_excl_maps() at the end of
check_and_resolve_insns(), after the final program digest is computed.
It also removes the early bpf_prog_calc_tag() call for signed programs
in bpf_check().

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5958425e9a2a5..14dacc5d202f7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20775,12 +20775,6 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
>  {
>  	enum bpf_prog_type prog_type = resolve_prog_type(prog);
>  
> -	if (map->excl_prog_sha &&
> -	    memcmp(map->excl_prog_sha, prog->digest, SHA256_DIGEST_SIZE)) {
> -		verbose(env, "program's hash doesn't match map's excl_prog_hash\n");
> -		return -EACCES;
> -	}
> -
>  	if (btf_record_has_field(map->record, BPF_LIST_HEAD) ||
>  	    btf_record_has_field(map->record, BPF_RB_ROOT)) {
>  		if (is_tracing_prog_type(prog_type)) {
> @@ -20878,6 +20872,29 @@ static int bpf_map_claim(struct bpf_verifier_env *env, struct bpf_map *map)
>  	}
>  }
>  
> +/*
> + * An exclusive map is bound to the digest of its program. The digest covers
> + * the insns as they are verified, after the CO-RE relocations the kernel
> + * applies, so it exists only once check_and_resolve_insns() has computed it,
> + * and the maps are checked against it there, whether an insn referred to
> + * them or a continuous fd_array bound them before.
> + */
> +static int check_excl_maps(struct bpf_verifier_env *env)
> +{
> +	int i;
> +
> +	for (i = 0; i < env->used_map_cnt; i++) {
> +		struct bpf_map *map = env->used_maps[i];
> +
> +		if (map->excl_prog_sha &&
> +		    memcmp(map->excl_prog_sha, env->prog->digest, SHA256_DIGEST_SIZE)) {
> +			verbose(env, "program's hash doesn't match map's excl_prog_hash\n");
> +			return -EACCES;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int __add_used_map(struct bpf_verifier_env *env, struct bpf_map *map)
>  {
>  	int i, err;

[Severity: High]
By moving the map->excl_prog_sha check out of check_map_prog_compatibility()
and deferring it to check_excl_maps() at the end of
check_and_resolve_insns(), can an unauthorized program with a non-matching
digest mutate map->user or BPF_MAP_TYPE_INSN_ARRAY state in __add_used_map()
before check_excl_maps() rejects it?

In kernel/bpf/verifier.c:__add_used_map(), bpf_map_claim() and
bpf_insn_array_init() now run before the exclusive hash check:

	err = check_map_prog_compatibility(env, map, env->prog);
	if (err)
		return err;
	...
	err = bpf_map_claim(env, map);
	if (err)
		return err;

	if (map->map_type == BPF_MAP_TYPE_INSN_ARRAY) {
		err = bpf_insn_array_init(map, env->prog);
		...
	}

If the legitimate owner program P1 has already claimed the exclusive map
(so map->user == (unsigned long)P1->aux) and a second program P2 with a
non-matching digest references the map via process_fd_array_continuous() or
a BPF_LD_IMM64 instruction, P2's call to bpf_map_claim() transitions
map->user to BPF_MAP_USER_MANY:

	if (cmpxchg(&map->user, old, old ? BPF_MAP_USER_MANY : me) == old)
		return 0;

When P2 is later rejected by check_excl_maps() with -EACCES,
release_maps() -> __bpf_free_used_maps() in kernel/bpf/core.c only resets
map->user if it still matches P2's aux:

	cmpxchg(&map->user, (unsigned long)aux, 0);

This leaves map->user permanently set to BPF_MAP_USER_MANY, causing
resolve_func_ptrs() in kernel/bpf/verifier.c to skip the map and
resolve_func_ptrs() in kernel/bpf/fixups.c to fail with -EBUSY for the
legitimate owner program. Similarly, if the exclusive map is a
BPF_MAP_TYPE_INSN_ARRAY, P2 will invoke bpf_insn_array_init() and claim
insn_array->used or reset its state before check_excl_maps() runs.

[ ... ]

> @@ -23564,11 +23581,6 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
>  		ret = -EINVAL;
>  		goto err_prep;
>  	}
> -	if (env->signature) {
> -		ret = bpf_prog_calc_tag(env->prog);
> -		if (ret < 0)
> -			goto err_prep;
> -	}
>  
>  	ret = process_fd_array(env, attr, uattr);
>  	if (ret)

[Severity: High]
Does removing the early digest check for signed programs allow
unauthenticated CO-RE relocations to modify env->prog->insnsi after
signature verification?

The commit message notes that the maps of a signed program are now checked
against the digest of the final instructions in check_excl_maps(), which
covers the relocations the kernel applies and the signature does not.

However, in kernel/bpf/verifier.c:bpf_check(), bpf_prog_verify_signature()
and security_bpf_prog_load() run on the initial unrelocated instructions
before bpf_prepare_btf_info() and bpf_check_core_relo():

	if (env->signature) {
		ret = bpf_prog_verify_signature(env, attr, uattr.is_kernel);
		if (ret)
			goto err_prep;
	}

	ret = security_bpf_prog_load(env->prog, attr, env->prog->aux->token,
				     uattr.is_kernel);
	if (ret)
		goto err_prep;
	...
	/* Prepare BTF and func_info needed to discover all subprograms. */
	ret = bpf_prepare_btf_info(env, attr, uattr);
	if (ret < 0)
		goto skip_full_check;

	/* Apply CO-RE before validating the program's instruction layout. */
	ret = bpf_check_core_relo(env, attr, uattr);
	if (ret < 0)
		goto skip_full_check;

In bpf_prog_verify_signature(), the PKCS#7 signature is verified only over
the initial prog->insnsi and the frozen map values, not over
map->excl_prog_sha (which is user-supplied via attr->excl_prog_hash during
BPF_MAP_CREATE), and a signed program with fd_array_cnt == 0 has no maps
checked in check_excl_maps() at all.

Because neither bpf_prepare_btf_info() nor bpf_check_core_relo() rejects
attr->prog_btf_fd or attr->core_relo_cnt when env->signature is set, could
a caller supply a validly signed program along with custom BTF and CO-RE
relocations (and maps created with attr->excl_prog_hash matching the
post-CO-RE digest) to mutate instructions in env->prog->insnsi after
bpf_prog_verify_signature() has already marked the program BPF_SIG_VERIFIED?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019032401.2302823-1-memxor@gmail.com?part=1