Re: [PATCH bpf-next 02/13] bpf: fetch the vmlinux BTF where kernel types enter a program

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bpf: NULL or ERR_PTR btf_vmlinux dereference via check_pseudo_btf_id()
- [Low] bpf: unreachable !btf check in check_ptr_to_map_access() when BTF is disabled
- [Low] bpf: unresolved TODO-NEEDS-IHOR Signed-off-by placeholder in commit log
--

commit a99de2b5b9828d221e9bcde23ac921cf3c9fc798
Author: Jay Wang <wanjay@amazon.com>

bpf: fetch the vmlinux BTF where kernel types enter a program

This commit drops the up-front vmlinux BTF fetch in bpf_check() and instead
fetches the vmlinux BTF on demand at the points where kernel types enter the
verifier state, avoiding unnecessary BTF parsing for programs that do not use
kernel types.

>     bpf: fetch the vmlinux BTF where kernel types enter a program
>     
>     bpf_check() fetches the vmlinux BTF up front for every program, whether
>     the program uses kernel types or not.  That is free today because the
>     BTF is parsed at boot anyway.  Once it can be parsed on first use
>     (CONFIG_DEBUG_INFO_BTF_LAZY, later in this series), it would make every
>     program load pay for parsing 5-7 MB of BTF, including the socket filters
>     systemd loads at boot.
>     
>     Drop the up-front fetch and fetch where kernel types enter the verifier
>     state instead:
>     
>      - bpf_add_kfunc_call(), for the first kfunc call of a program;
>      - check_ptr_to_map_access(), for map_ptr field access;
>      - check_pseudo_btf_id(), for ldimm64 of a kernel variable (ksym);
>      - check_helper_call(), for helpers whose prototype takes
>        ARG_PTR_TO_BTF_ID or returns RET_PTR_TO_BTF_ID, and for
>        bpf_snprintf_btf() and bpf_seq_printf_btf(), which take a kernel
>        type id inside struct btf_ptr (helper_uses_vmlinux_btf()).
>     
>     The other ways in already fetch the BTF themselves: attach_btf in
>     bpf_prog_load(), struct_ops map creation, CO-RE candidate lookup,
>     bpf_find_btf_id(), btf_prepare_func_args() for kernel-typed arguments
>     and the kernel context type lookups.  The remaining reads of the
>     btf_vmlinux global in the verifier are on paths dominated by one of
>     these fetches.  A program that uses none of them, such as a socket
>     filter, does not need the vmlinux BTF any more, and verifies the same
>     whether or not it has been parsed.
>     
>     Four related changes keep it that way:
>     
>      - The "in-kernel BTF is malformed" check in bpf_check() read the
>        global right after the up-front fetch.  Without the fetch it would
>        pass or fail depending on whether something else had parsed the BTF
>        before, so drop it; every fetch point reports a missing BTF itself
>        and passes on an error of the parse, as that check did.
>     
>      - check_ptr_to_map_access() checks allow_ptr_leaks before it fetches
>        the BTF, and bpf_prog_load() falls back to the vmlinux BTF for an
>        attach_btf_id only with CAP_BPF, which all program types that take
>        one need, so that an unprivileged load cannot make the kernel parse
>        it only to be refused.  The map_ptr permission error can then not
>        name the map's struct any more and says "map_ptr access is allowed
>        only to CAP_PERFMON and CAP_SYS_ADMIN".  A ksym, which unprivileged
>        programs may use, still parses it.
>     
>      - Add bpf_peek_btf_vmlinux(), which returns the vmlinux BTF only if it
>        is parsed already, and use it in bpf_btf_printf_prepare().  The BTF
>        printf helpers run in program context, which may be atomic, and the
>        verifier has fetched the BTF for every program that calls them.
>     
>      - is_kfunc_dynptr_write(), reached from the argument checks that
>        helpers and kfuncs share, compared meta->btf with the global, which
>        a helper's NULL matches while the global is NULL.  Check that the
>        call is a kfunc first, as the other kfunc predicates there do.
>     
>     Helpers taking ARG_PTR_TO_BTF_ID_SOCK_COMMON are left out on purpose:
>     they take sockets without BTF types too, the shared argument check reads
>     the global only for a PTR_TO_BTF_ID register, and every source of such
>     a register fetched the BTF.
>     
>     No functional change with CONFIG_DEBUG_INFO_BTF=y, where kfunc
>     registrations at boot parse the BTF, other than that error message and
>     that, if the vmlinux BTF fails to parse (a broken toolchain), programs
>     that use no kernel types now load instead of failing.  Without BTF the
>     new helper check is compiled out and the existing error paths are
>     unchanged.
>     
>     Co-developed-by: Ihor Solodrai <ihor.solodrai@linux.dev>
>     TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
>     Signed-off-by: Jay Wang <wanjay@amazon.com>

[Severity: Low]
This isn't a bug in the code, but the commit log still has an unresolved
TODO-NEEDS-IHOR prefix on the Signed-off-by trailer following
Co-developed-by. Should that placeholder be resolved before merging?

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5601d20562186..1b901e3cb3c22 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -6867,30 +6883,33 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
>  	enum bpf_type_flag flag = 0;
>  	const struct btf_type *t;
>  	const char *tname;
> +	struct btf *btf;
>  	u32 btf_id;
>  	int ret;
>  
> -	if (!btf_vmlinux) {
> -		verbose(env, "map_ptr access not supported without CONFIG_DEBUG_INFO_BTF\n");
> -		return -ENOTSUPP;
> -	}
> -
>  	if (!map->ops->map_btf_id || !*map->ops->map_btf_id) {
>  		verbose(env, "map_ptr access not supported for map type %d\n",
>  			map->map_type);
>  		return -ENOTSUPP;
>  	}
>  
> -	t = btf_type_by_id(btf_vmlinux, *map->ops->map_btf_id);
> -	tname = btf_name_by_offset(btf_vmlinux, t->name_off);
> -
> +	/* before the BTF fetch: unprivileged loads must not parse it */
>  	if (!env->allow_ptr_leaks) {
> -		verbose(env,
> -			"'struct %s' access is allowed only to CAP_PERFMON and CAP_SYS_ADMIN\n",
> -			tname);
> +		verbose(env, "map_ptr access is allowed only to CAP_PERFMON and CAP_SYS_ADMIN\n");
>  		return -EPERM;
>  	}
>  
> +	btf = bpf_get_btf_vmlinux();
> +	if (IS_ERR(btf))
> +		return vmlinux_btf_err(env, btf);
> +	if (!btf) {
> +		verbose(env, "map_ptr access not supported without CONFIG_DEBUG_INFO_BTF\n");
> +		return -ENOTSUPP;
> +	}

[Severity: Low]
Can this !btf check in check_ptr_to_map_access() ever be reached?

When CONFIG_DEBUG_INFO_BTF is disabled, BTF_ID_LIST_SINGLE() and
BTF_ID_LIST_GLOBAL_SINGLE() in include/linux/btf_ids.h define zero-initialized
arrays:

    #define BTF_ID_LIST_SINGLE(name, prefix, typename) static u32 __maybe_unused name[1];
    #define BTF_ID_LIST_GLOBAL_SINGLE(name, prefix, typename) u32 __maybe_unused name[1];

Because *map->ops->map_btf_id is 0 for all map types when
CONFIG_DEBUG_INFO_BTF is disabled, moving bpf_get_btf_vmlinux() after the
!*map->ops->map_btf_id check causes check_ptr_to_map_access() to always exit
early with "map_ptr access not supported for map type %d", leaving the !btf
branch unreachable.

[ ... ]

> @@ -20898,12 +20941,14 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
>  			return -EINVAL;
>  		}
>  	} else {
> -		if (!btf_vmlinux) {
> +		btf = bpf_get_btf_vmlinux();
> +		if (IS_ERR(btf))
> +			return vmlinux_btf_err(env, btf);
> +		if (!btf) {
>  			verbose(env, "kernel is missing BTF, make sure CONFIG_DEBUG_INFO_BTF=y is specified in Kconfig.\n");
>  			return -EINVAL;
>  		}
> -		btf_get(btf_vmlinux);
> -		btf = btf_vmlinux;
> +		btf_get(btf);
>  	}
>  
>  	err = __check_pseudo_btf_id(env, insn, aux, btf);

[Severity: High]
Can a program create a PTR_TO_BTF_ID register through check_pseudo_btf_id()
without btf_vmlinux being fetched or validated, leading to a NULL or ERR_PTR
dereference of btf_vmlinux in check_func_arg()?

In check_helper_call(), bpf_get_btf_vmlinux() is only called when
IS_ENABLED(CONFIG_DEBUG_INFO_BTF) && helper_uses_vmlinux_btf(func_id, fn), and
helper_uses_vmlinux_btf() omits ARG_PTR_TO_BTF_ID_SOCK_COMMON under the
assumption that every source of a PTR_TO_BTF_ID register has already fetched
btf_vmlinux.

However, in check_pseudo_btf_id(), when btf_fd (insn[1].imm) is non-zero:

    btf_fd = insn[1].imm;
    if (btf_fd) {
        btf = btf_get_by_fd(btf_fd);
        if (IS_ERR(btf)) {
            verbose(env, "invalid module BTF object FD specified.\n");
            return -EINVAL;
        }
    } else {
        btf = bpf_get_btf_vmlinux();
        ...
    }

btf_get_by_fd() accepts any BTF fd, including a user-loaded BTF created via
BPF_BTF_LOAD (which does not fetch btf_vmlinux), and check_pseudo_btf_id()
does not verify btf_is_module(btf) or btf_is_kernel(btf). If a user loads a
BTF containing a BTF_KIND_STRUCT and a BTF_KIND_VAR whose name matches a
kernel symbol in kallsyms, __check_pseudo_btf_id() sets
aux->btf_var.reg_type = PTR_TO_BTF_ID and check_ld_imm() marks the destination
register as PTR_TO_BTF_ID while btf_vmlinux is still NULL (with
CONFIG_DEBUG_INFO_BTF_LAZY=y before first parse, or CONFIG_DEBUG_INFO_BTF=n)
or ERR_PTR (if btf_parse_vmlinux() failed).

When that PTR_TO_BTF_ID register is then passed to a helper taking
ARG_PTR_TO_BTF_ID_SOCK_COMMON (or ARG_PTR_TO_BTF_ID when
CONFIG_DEBUG_INFO_BTF=n), check_func_arg() does:

    case ARG_PTR_TO_BTF_ID:
    case ARG_PTR_TO_BTF_ID_SOCK_COMMON:
    {
        const u32 *arg_btf_id = fn->arg_btf_id[arg];
        const struct btf *arg_btf = is_kfunc(meta) ? meta->btf : btf_vmlinux;
        ...
        if (!meta->btf) {
            ...
            if (base_type(reg->type) != PTR_TO_BTF_ID)
                break;
        }
        ...
        if (!process_arg_ptr_to_btf_id(env, reg, regno, arg_btf_id, arg_btf, meta))

which passes arg_btf = btf_vmlinux (NULL or ERR_PTR) into
bpf_diag_fmt_btf_type() and process_arg_ptr_to_btf_id() ->
btf_type_skip_modifiers() -> btf_type_by_id(), dereferencing arg_btf.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031130959.23472-1-wanjay@amazon.com?part=2