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 <[email protected]> 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 <[email protected]> > TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <[email protected]> > Signed-off-by: Jay Wang <[email protected]> [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/[email protected]?part=2
