Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] bpftool: buffer over-read of uninitialized heap memory in sym_name_trim_prefix() - [Medium] bpf: strict fids check in bpf_tracing_multi_link_fill_link_info() breaks UAPI --
commit 69edd76aa1ef54260b1403707834fa758ec91952 Author: Leon Hwang <[email protected]> bpf: Add tracing_multi link info support for bpf progs This commit adds support for showing BPF program information in tracing_multi links via bpftool. > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 8b1a52350cfda..39c911917ae24 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c [ ... ] > @@ -3725,9 +3726,13 @@ static int bpf_tracing_multi_link_fill_link_info(const > struct bpf_link *link, > info->tracing_multi.attach_type = tr_link->link.attach_type; > info->tracing_multi.count = tr_link->nodes_cnt; > info->tracing_multi.btf_obj_id = > btf_obj_id(tr_link->link.prog->aux->attach_btf); > + has_progs = !!tr_link->tgt_progs; > + info->tracing_multi.flags = has_progs ? BPF_F_TRACING_MULTI_TGT_PROGS : > 0; > > - if (!uids && !ucookies && !uaddrs) > + if (!uids && !ucookies && !uaddrs && !fids) > return 0; > + if (has_progs != !!fids) > + return -EINVAL; [Severity: Medium] Does this strict equality check break forward compatibility for generic userspace tools? If a tool unconditionally pre-allocates and provides a func_btf_ids array while querying an older, non-BPF-targeted tracing_multi link (where has_progs is false), bpf_obj_get_info_by_fd() will fail with -EINVAL. Would it be safer to gracefully ignore or zero-fill the fids buffer if the target does not have programs, rather than rejecting the query entirely? > diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c > index 5cffa2d283b47..da7e430b14c8a 100644 > --- a/tools/bpf/bpftool/link.c > +++ b/tools/bpf/bpftool/link.c [ ... ] > @@ -422,12 +422,31 @@ show_uprobe_multi_json(struct bpf_link_info *info, > json_writer_t *wtr) > jsonw_end_array(json_wtr); > } > > +#define BPF_PROG_PREFIX "bpf_prog_" > +#define BPF_PROG_PREFIX_LEN (sizeof(BPF_PROG_PREFIX) - 1) > + > +static const char * > +sym_name_trim_prefix(struct kernel_sym *sym, bool is_prog) > +{ > + int prefix_len; > + > + if (!is_prog) > + return sym->name; > + > + /* Ref kernel/bpf/core.c::bpf_prog_ksym_set_name() */ > + prefix_len = BPF_PROG_PREFIX_LEN; > + prefix_len += BPF_TAG_SIZE * 2; > + if (sym->name[prefix_len] != '\0') [Severity: Medium] Can this result in an out-of-bounds read if the kernel symbol name is shorter than the prefix length? In bpftool, sym->name is allocated using libbpf_reallocarray() and populated via sscanf(), which null-terminates the string but leaves the remainder of the buffer uninitialized. If a target address resolves to a short kernel symbol (for example, due to a TOCTOU race where a BPF program is unloaded and its vmalloc address is reused), sym->name[prefix_len] evaluates uninitialized heap memory. If that byte happens to be non-zero, it skips it and returns a pointer into the uninitialized region, potentially corrupting CLI or JSON output. Should we verify that strlen(sym->name) is sufficiently large before checking the character at prefix_len? > + prefix_len += 1; /* skip the '_' */ > + return sym->name + prefix_len; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
