Re: [PATCH bpf-next v4 5/6] libbpf: Add log_opts support for bpf_link_create

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]

> 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?

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.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40108748084