On 2026/10/25 00:13, [email protected] wrote: >> libbpf: Add log_opts support for bpf_link_create > > TL;DR: In bpf_link_create(), the shared iter_info/log_opts slot is keyed only > on iter_info_len, so an iter_info pointer with len 0 may be misread as > log_opts, possibly causing out-of-bounds reads/writes in libbpf. > >> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c >> index 9a2107d4f6d8..00efa90a6ed9 100644 >> --- a/tools/lib/bpf/bpf.c >> +++ b/tools/lib/bpf/bpf.c > > [ ... ] > >> @@ -803,6 +805,14 @@ int bpf_link_create(int prog_fd, int target_fd, >> iter_info_len = OPTS_GET(opts, iter_info_len, 0); >> target_btf_id = OPTS_GET(opts, target_btf_id, 0); >> >> + if (iter_info_len && attach_type != BPF_TRACE_ITER) >> + return libbpf_err(-EINVAL); >> + >> + if (iter_info_len) >> + iter_info = OPTS_GET(opts, iter_info, (void *)0); >> + else >> + log_opts = OPTS_GET(opts, log_opts, (void *)0); >> + >> /* validate we don't have unexpected combinations of non-zero fields */ >> if (iter_info_len || target_btf_id) { >> if (iter_info_len && target_btf_id) > > For BPF_TRACE_ITER, iter_info_len is now the only thing that decides whether > the shared iter_info/log_opts slot holds a union bpf_iter_link_info pointer > or a struct bpf_log_opts pointer. > > Can a caller that sets opts->iter_info but leaves opts->iter_info_len at 0 > end up with its union bpf_iter_link_info being read as a struct > bpf_log_opts here?
Users should not do like this. By convention, iter_info_len==0 means no iter_info. > > bpf_program__attach_iter() copies opts->link_info into > link_create_opts.iter_info and opts->link_info_len into iter_info_len > without checking that they agree, and nothing in bpf.h or libbpf.h says > log_opts is only used when iter_info_len == 0, or that iter_info needs > iter_info_len. > > Before this patch the pointer went to the kernel, and bpf_iter_link_attach() > rejected the mismatch cleanly: > > if (bpfptr_is_null(ulinfo) ^ !linfo_len) > return -EINVAL; > > With this patch, __sys_bpf_fd() runs OPTS_VALID(log_opts, bpf_log_opts) on > the 16-byte union bpf_iter_link_info. The first 8 bytes of that union > (map.map_fd, or task.tid/pid, or cgroup.order/cgroup_fd) become > log_opts->sz: > > - If map_fd or tid is in the range 8..27, sz passes validation, and size and > level are read from bytes 16..24, which lie past the end of the 16-byte > object. > > - If sz is 28 to 31, OPTS_HAS(log_opts, true_size) is true, so > OPTS_SET(log_opts, true_size, ...) writes 4 bytes at offset 24, which is 8 > bytes past the end of the caller's object. Can this corrupt the caller's > stack or heap? > > - If sz is larger (a typical tid, or a cgroup fd shifted into the high 32 > bits), libbpf_is_mem_zeroed() scans caller memory from offset 28 up to sz. > It usually stops with a "bpf_log_opts has non-zero extra bytes" warning, > but it reads out of bounds first. > > In addition, attr.link_create.iter_info is now NULL with iter_info_len of 0, > which the kernel accepts. If validation does pass, could a task iterator > attach to all tasks instead of the requested tid, and a map iterator fail > with -EBADF instead of -EINVAL? > > The commit message says log_opts support for iter links is future work. > Would it be better to treat the slot as iter_info whenever > attach_type == BPF_TRACE_ITER, and read log_opts only for other attach > types, instead of keying only on iter_info_len? That would keep the old > behaviour for iter links exactly. Not exactly. iter_info and iter_info_len must be set at the same time for iter links. If iter_info_len is 0, iter_info must be NULL or zeroed memory. Users must not misuse them by iter_info_len==0 but iter_info is not zeroed memory. Thanks, Leon
