Re: [PATCH bpf-next v2 1/5] bpf: Verify signed loader metadata at load time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] TOCTOU race in BPF signed program verification allows bypassing signature via concurrent modification of user-space fd_array.
--

commit e61d684bd0467f62e035ea0592a8066b9c7246fb
Author: Daniel Borkmann <daniel@iogearbox.net>

bpf: Verify signed loader metadata at load time

This patch modifies the BPF verifier to perform integrity checking of a signed
program's metadata array maps before the LSM admission hook fires. It achieves
this by resolving the maps from the fd_array, folding their frozen contents
into the signature verification process, and validating the map count.

Link: https://lore.kernel.org/bpf/CAHC9VhSDkwGgPfrBUh7EgBKEJj_JjnY68c0YAmuuLT_i--GskQ@mail.gmail.com [0]
Link: https://lore.kernel.org/bpf/2f71d6c03698eb17d51f7247efde777627ee578a.camel@HansenPartnership.com [1]
Link: https://lore.kernel.org/lkml/ecf0521ed302db672672ebfbc670ecfba36a6e00.camel@HansenPartnership.com [2]
Link: https://lore.kernel.org/bpf/88703f00d5b7a779728451008626efa45e42db3d.camel@HansenPartnership.com [3]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2abc79dbf281c..9cd2b62da3803 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -19895,11 +19895,28 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
>  	ret = bpf_vlog_init(&env->log, attr_log->level, attr_log->ubuf, attr_log->size);
>  	if (ret)
>  		goto err_unlock;
> +	if (env->check_signature) {
> +		ret = bpf_prog_calc_tag(env->prog);
> +		if (ret < 0)
> +			goto skip_full_check;
> +	}
>  
>  	ret = process_fd_array(env, attr, uattr);
>  	if (ret)
>  		goto skip_full_check;
>  
> +	if (env->check_signature) {
> +		ret = bpf_prog_verify_signature(env, attr, uattr.is_kernel);
> +		if (ret)
> +			goto skip_full_check;
> +		signed_map_cnt = env->used_map_cnt;
> +	}

[ ... ]

> @@ -19966,7 +19966,14 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
>  	ret = check_and_resolve_insns(env);
>  	if (ret < 0)
>  		goto skip_full_check;
> -
> +	if (env->prog->aux->sig.verdict == BPF_SIG_VERIFIED &&
> +	    (env->used_map_cnt != signed_map_cnt || env->used_btf_cnt)) {

[Severity: High]
Is there a TOCTOU race with how fd_array is processed between these checks?

If an attacker concurrently modifies the fd_array in user memory after
process_fd_array() runs but before check_and_resolve_insns() reads it again:

1. process_fd_array() reads the array from user space and populates
   env->used_maps with MAP_A and MAP_B.
2. bpf_prog_verify_signature() successfully verifies the signature over
   these specific maps, and signed_map_cnt is set to 2.
3. The attacker modifies the user-space fd_array memory (e.g., swapping the
   FDs for MAP_A and MAP_B, or duplicating MAP_B).
4. check_and_resolve_insns() reads the modified fd_array from user space
   again via copy_from_bpfptr_offset(&fd, env->fd_array, ...) and binds the
   swapped maps to the program's pseudo instructions.
5. This final integrity check (env->used_map_cnt != signed_map_cnt) still
   passes because the total map count remains unchanged, but the
   cryptographic bindings have been completely bypassed.

Would it be safer to cache the fd_array in kernel memory during the initial
process_fd_array() call so that check_and_resolve_insns() does not need to
read from user space a second time?

> +		verbose(env, "signed program uses %s not covered by the signature\n",
> +			env->used_map_cnt != signed_map_cnt ?
> +			(env->used_btf_cnt ? "maps and BTF" : "maps") : "BTF");
> +		ret = -EACCES;
> +		goto skip_full_check;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624140301.93421-1-daniel@iogearbox.net?part=1