Re: [PATCH bpf-next v5 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 1 potential issue(s) to consider:
- [High] bpf: unauthenticated CO-RE relocations after bpf_prog_verify_signature()
--

commit 32dc3dc6de06a5dac6e79c2b94b85778ecf062a2
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Check exclusive maps once the program digest is final

This commit defers the exclusive map hash comparison and map claiming until
check_and_resolve_insns() has computed the program digest after CO-RE
relocations. It also removes the early bpf_prog_calc_tag() call for signed
programs before process_fd_array().

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d138688468236..ca5301fe3f66c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -23619,11 +23657,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]
Can removing the pre-relocation digest check allow unauthenticated CO-RE
relocations to mutate a signed program's instructions after
bpf_prog_verify_signature() and security_bpf_prog_load() have succeeded?

In bpf_check(), bpf_prog_verify_signature() verifies the PKCS#7 signature
over the pre-relocation env->prog->insnsi and the frozen map value contents
before BTF and CO-RE relocations are processed:

    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);
    ...
    /* 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;

Although fd_array_get_btf() and __add_used_btf() reject BTF objects from
fd_array when env->signature is set, bpf_prepare_btf_info() loads
attr->prog_btf_fd via btf_get_by_fd() without checking env->signature, and
bpf_check_core_relo() applies caller-supplied attr->core_relos to
env->prog->insnsi in-place.

Furthermore, in bpf_prog_verify_signature(), the signature only covers
env->prog->insnsi and the map values, not map->excl_prog_sha:

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

The 32-byte hash stored in map->excl_prog_sha is supplied by userspace at
BPF_MAP_CREATE time via attr->excl_prog_hash (and a signed program with
env->used_map_cnt == 0 has no exclusive map at all).

Previously, for a signed program with an exclusive map, bpf_prog_calc_tag()
computed env->prog->digest over the signed instructions before
process_fd_array(), and check_map_prog_compatibility() required
map->excl_prog_sha to match that pre-relocation digest.

With this change, check_excl_maps() only compares map->excl_prog_sha against
env->prog->digest after bpf_check_core_relo() has already mutated the
instructions:

    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;
    }

If a caller takes a validly signed program and metadata, supplies a crafted
prog_btf_fd and core_relos (such as BPF_CORE_TYPE_ID_LOCAL, where
bpf_core_calc_relo_insn() sets targ_res->validate = false and
bpf_core_patch_insn() overwrites insn->imm or insn->off without validating
the original immediate or offset), and creates the exclusive map with
excl_prog_hash set to the SHA-256 of the post-relocation instructions,
wouldn't both bpf_prog_verify_signature() and check_excl_maps() succeed while
loading modified instructions with BPF_SIG_VERIFIED?

Should signed programs either reject attr->core_relos_cnt (or prog_btf_fd) or
ensure that env->prog->insnsi cannot be modified after
bpf_prog_verify_signature()?

 base-commit: 25bb03f0a47b622b9025a1558bf2095ec1f55d26
-- 
2.53.0.1073.g0031eab094-goog

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