Re: [PATCH bpf-next v5 2/8] 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:

Pre-existing issues:
- [High] A lockless synchronization race between map_freeze() and map_update_elem() allows a TOCTOU bypass of BPF signature verification.
--

--- Patch [2]: [PATCH bpf-next v5 2/8] bpf: Verify signed loader metadata at load time ---
commit 5bc066f1169723d401bd2b1b6a80fa2ecfbca1a9
Author: Daniel Borkmann <daniel@iogearbox.net>
Subject: bpf: Verify signed loader metadata at load time

This patch introduces load-time BPF signature verification. It verifies the
signature over both the BPF instructions and the contents of any exclusive
metadata maps, relying on map->frozen to ensure the metadata cannot be
modified after verification.

Link: https://lore.kernel.org/bpf/2f71d6c03698eb17d51f7247efde777627ee578a.camel@HansenPartnership.com [0]
Link: https://lore.kernel.org/lkml/ecf0521ed302db672672ebfbc670ecfba36a6e00.camel@HansenPartnership.com [1]
Link: https://lore.kernel.org/bpf/88703f00d5b7a779728451008626efa45e42db3d.camel@HansenPartnership.com [2]
Link: https://lore.kernel.org/bpf/DJOFY21DYUI4.19WKQ3NPZ4H5R@gmail.com [3]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2e9e65297c706..462a1f8544b9d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19870,6 +19895,140 @@ int bpf_fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	return 0;
>  }
>  
[ ... ]
> +static int bpf_prog_verify_signature(struct bpf_verifier_env *env,
> +				     union bpf_attr *attr, bool is_kernel)
> +{
[ ... ]
> +	for (i = 0; i < map_cnt; i++) {
> +		struct bpf_map *map = maps[i];
> +
> +		if (map->map_type != BPF_MAP_TYPE_ARRAY ||
> +		    !map->ops->map_direct_value_addr) {
> +			verbose(env, "signed program metadata map '%s' must be an array\n",
> +				map->name);
> +			err = -EINVAL;
> +			goto out;
> +		}
> +		if (!READ_ONCE(map->frozen)) {

[Severity: High]
This is a pre-existing issue, but does the current map freezing mechanism
provide the absolute immutability required here?

In kernel/bpf/syscall.c:map_update_elem(), the code increments the write
count and checks permissions:

    bpf_map_write_active_inc(map);
    if (!(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) {

Because bpf_map_write_active_inc() uses atomic64_inc(), it does not imply a
memory barrier on weakly-ordered architectures (like ARM64). Could the
read of map->frozen inside map_get_sys_perms() be reordered to occur
before the writecnt increment is globally visible?

If so, could a sequence like this occur?

CPU0 (map_update_elem):
    bpf_map_write_active_inc(map); // no barrier
    // map->frozen read reordered here, sees false
    // blocks on user memory fault during kvmemdup_bpfptr()

CPU1 (map_freeze):
    // reads writecnt as 0, assumes no active writers
    sets map->frozen = true;

CPU2 (bpf_prog_verify_signature):
    reads map->frozen as true, verifies signature against benign data

CPU0 (map_update_elem):
    resumes from memory fault, completes update with new data since
    bpf_map_update_value() doesn't re-check map->frozen

Since this patch relies on map->frozen to guarantee the metadata cannot
change, does this lockless race allow bypassing the signature verification?

> +			verbose(env, "signed program metadata map '%s' must be frozen\n",
> +				map->name);
> +			err = -EPERM;
> +			goto out;
> +		}
> +		if (!map->excl_prog_sha) {
> +			verbose(env, "signed program metadata map '%s' must be exclusive\n",
> +				map->name);
> +			err = -EPERM;
> +			goto out;
> +		}
> +		data_sz += map->value_size;
> +	}

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