Re: [PATCH bpf-next 5/9] libbpf: Add log_opts support for BPF_TRACE_ITER

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] libbpf: missing OPTS_ZEROED() check for BPF_TRACE_ITER in
  bpf_link_create()
- [Low] libbpf: unreachable iter.len check in bpf_link_create()
- [Low] libbpf: deprecated iter_info fields used in
  bpf_program__attach_iter()
--

commit 254d03820df80f0d29b270cec3be5cafe9178fe6
Author: Leon Hwang <leon.hwang@linux.dev>

libbpf: Add log_opts support for BPF_TRACE_ITER

This commit adds iter.info and iter.len to struct bpf_link_create_opts so
that BPF_TRACE_ITER link creation can pass iterator parameters alongside
log_opts, while keeping the legacy iter_info and iter_info_len fields for
callers that do not use log_opts.

> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 38dcb31e8c0ed..174a8bfaf6638 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -808,11 +808,6 @@ int bpf_link_create(int prog_fd, int target_fd,
>  	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)
> @@ -821,6 +816,16 @@ int bpf_link_create(int prog_fd, int target_fd,
>  			return libbpf_err(-EINVAL);
>  	}
>  
> +	if (iter_info_len) {
> +		if (OPTS_GET(opts, iter.len, 0))
> +			return libbpf_err(-EINVAL);

[Severity: Low]
Can OPTS_GET(opts, iter.len, 0) ever be non-zero here?  When iter_info_len
is set, the preceding if (iter_info_len || target_btf_id) block has already
verified OPTS_ZEROED(opts, target_btf_id), which rejects any non-zero bytes
in the union following target_btf_id, including opts->iter.len.

> +		iter_info = OPTS_GET(opts, iter_info, (void *)0);
> +	} else {
> +		log_opts = OPTS_GET(opts, log_opts, (void *)0);
> +		iter_info = OPTS_GET(opts, iter.info, (void *)0);
> +		iter_info_len = OPTS_GET(opts, iter.len, 0);
> +	}

[Severity: Low]
Should bpf_program__attach_iter() also be updated to populate
link_create_opts.iter.info and link_create_opts.iter.len instead of the
deprecated iter_info and iter_info_len fields?

tools/lib/bpf/libbpf.c:bpf_program__attach_iter() {
    ...
	link_create_opts.iter_info = OPTS_GET(opts, link_info, (void *)0);
	link_create_opts.iter_info_len = OPTS_GET(opts, link_info_len, 0);
    ...
}

Because iter_info shares a union with log_opts, calling
bpf_program__attach_iter() with link_info != NULL and link_info_len == 0
causes bpf_link_create() to take the else branch above and dereference the
union bpf_iter_link_info pointer as struct bpf_log_opts *log_opts in
__sys_bpf_fd().

[ ... ]

> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index fe3b7fd8512bd..2ac143fd1102e 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h

[ ... ]

> @@ -478,6 +482,10 @@ struct bpf_link_create_opts {
>  			const __u64 *cookies;
>  			__u32 cnt;
>  		} tracing_multi;
> +		struct {
> +			union bpf_iter_link_info *info;
> +			__u32 len;
> +		} iter;
>  	};
>  	size_t :0;
>  };

[Severity: Low]
Does case BPF_TRACE_ITER in bpf_link_create() miss an OPTS_ZEROED(opts,
iter) check?

tools/lib/bpf/bpf.c:bpf_link_create() {
    ...
	switch (attach_type) {
	case BPF_TRACE_ITER:
		attr.link_create.iter_info = ptr_to_u64(iter_info);
		attr.link_create.iter_info_len = iter_info_len;
		break;
    ...
}

When opts->iter is used (so iter_info_len and target_btf_id are 0),
trailing bytes in the anonymous union after opts->iter overlapping larger
members such as uprobe_multi are not checked for zero, unlike all other
union members in the switch statement.  Also, opts->iter.info and
opts->iter.len are read in the !iter_info_len branch even when attach_type
is not BPF_TRACE_ITER.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109140816.5196-1-leon.hwang@linux.dev?part=5