Re: [PATCH bpf-next v4 3/9] bpf: Verify signed loader metadata at load time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Moving `security_bpf_prog_load` into `bpf_check` causes the `bpf_prog_free` LSM hook to be executed twice if the load hook fails, or executed without a prior allocation/load hook if `bpf_check` fails early.

Pre-existing issues:
- [High] bpf_prog_calc_tag() fails to mask out kernel addresses for BPF_PSEUDO_MAP_IDX and BPF_PSEUDO_MAP_IDX_VALUE instructions, leaking kernel heap addresses into the subprogram tag.
--

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

bpf: Verify signed loader metadata at load time

This commit modifies the BPF loader to verify PKCS#7 signatures over
the loader instructions and its metadata maps. It also moves the
security_bpf_prog_load() admission hook into bpf_check(), running it
after the signature is verified but before full 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 e227c76a8dd96..65f53128fc33d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -19909,11 +20068,27 @@ 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_free_env;
> +	if (env->signature) {
> +		ret = bpf_prog_calc_tag(env->prog);

[Severity: High]
This is a pre-existing issue, but does bpf_prog_calc_tag() fail to mask out
kernel addresses for BPF_PSEUDO_MAP_IDX and BPF_PSEUDO_MAP_IDX_VALUE
instructions?

Looking at bpf_prog_calc_tag() in kernel/bpf/core.c, it only masks the
immediate fields for BPF_PSEUDO_MAP_FD and BPF_PSEUDO_MAP_VALUE:

kernel/bpf/core.c:bpf_prog_calc_tag() {
    ...
    if (!was_ld_map &&
        dst[i].code == (BPF_LD | BPF_IMM | BPF_DW) &&
        (dst[i].src_reg == BPF_PSEUDO_MAP_FD ||
         dst[i].src_reg == BPF_PSEUDO_MAP_VALUE)) {
        was_ld_map = true;
        dst[i].imm = 0;
    ...
}

During check_and_resolve_insns() in kernel/bpf/verifier.c, the actual kernel
address is written into the imm fields for these instructions. Could this
cause the unmasked kernel heap addresses of BPF maps to be incorporated
into the SHA256 tag of BPF subprograms, allowing a local attacker to
recover the pointer and bypass KASLR?

> +		if (ret < 0)
> +			goto err_prep;
> +	}
>  
>  	ret = process_fd_array(env, attr, uattr);
>  	if (ret)
>  		goto err_prep;
>  
> +	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;

[Severity: High]
If this hook fails, does it result in security_bpf_prog_free() being called
twice, or executed on an uninitialized state?

When bpf_check() is called from bpf_prog_load() in kernel/bpf/syscall.c, a
failure routes to the free_used_maps label, which unconditionally calls
__bpf_prog_put_noref() and subsequently security_bpf_prog_free() via
__bpf_prog_put_rcu():

kernel/bpf/syscall.c:bpf_prog_load() {
    ...
    err = bpf_check(&prog, attr, uattr, attr_log);
    if (err < 0)
        goto free_used_maps;
    ...
free_used_maps:
    __bpf_prog_put_noref(prog, prog->aux->real_func_cnt);
    return err;
}

Since the security_bpf_prog_load hook frees the LSM state internally on
failure, the resulting error return causes the caller to free it a second
time. Alternatively, if bpf_check() fails early (like in process_fd_array),
this is bypassed entirely, causing a free without the load hook ever
running.

Can this corrupt external reference counts or state maps for BPF LSM
programs attached to this hook?

> +
>  	bpf_get_btf_vmlinux();
>  
>  	/* grab the mutex to protect few globals used by verifier */

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