Re: [RFC PATCH bpf-next 09/12] bpf: Add kernel support to load SDT observer program

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Reference count leak of `dst_prog` due to pointer overwrite in `bpf_prog_load()`.
- [High] Error paths in `BPF_TRACE_SDT` handling bypass `put_sdt` cleanup, leaking `attach_btf` and `dst_prog`.
- [High] Memory leak of `prog->aux->sdt_probe` on early load failures in `bpf_prog_load()`.
- [Medium] bpf_insn_array_get_sdt_probe_by_name returns an incorrect semantic error code when a probe is not found.
--

commit 02476bf5b9d80f3ff0471539243d877d096c0806
Author: Xu Kuohai <xukuohai@huawei.com>

bpf: Add kernel support to load SDT observer program

Add kernel support to load SDT observer program. The program is
verified as a normal tracing type program while the target FUNC_PROTO
is resolved in the SDT map of the target program using the probe name
passed by the user.

> diff --git a/kernel/bpf/bpf_insn_array.c b/kernel/bpf/bpf_insn_array.c
> index a9564ef4b2efa..067cc1b817d5f 100644
> --- a/kernel/bpf/bpf_insn_array.c
> +++ b/kernel/bpf/bpf_insn_array.c
[ ... ]
> +int bpf_insn_array_get_sdt_probe_by_name(struct bpf_prog *prog, const char *name,
> +					 struct bpf_insn_array_value *val,
> +					 unsigned long *ip)
> +{
[ ... ]
> +		for (j = 0; j < map->max_entries; j++) {
> +			if (insn_array->values[j].xlated_off == INSN_DELETED)
> +				continue;
> +			if (!strcmp(insn_array->values[j].name, name)) {
> +				*val = insn_array->values[j];
> +				*ip = (unsigned long)insn_array->ips[j];
> +				return 0;
> +			}
> +		}
> +	}
> +
> +	return -EEXIST;

[Severity: Medium]
Is -EEXIST the correct error code when a probe is not found? Would -ENOENT be
more appropriate to indicate a missing entry?

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index dc881e5ad4114..8b2c73bb6c2a8 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
[ ... ]
> @@ -3082,33 +3085,75 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, struct bpf_log_at
>  		btf_get(attach_btf);
>  	}
>  
> +	/*
> +	 * find the probe site in the target program at load time of the
> +	 * observer program, so the verifier can check the observer's context
> +	 * arguments from the probe's FUNC_PROTO.
> +	 */
> +	if (attr->expected_attach_type == BPF_TRACE_SDT) {
> +		struct bpf_insn_array_value sdt_val;
> +		struct bpf_sdt_probe_info *sdt_probe;
> +		unsigned long probe_ip;
> +
> +		if (!bpf_jit_supports_sdt_probe()) {
> +			err = -EOPNOTSUPP;
> +			goto put_token;
> +		}
> +		if (!attr->sdt.target_prog_fd) {
> +			err = -EINVAL;
> +			goto put_token;

[Severity: High]
Does jumping to put_token here bypass the cleanup of attach_btf and dst_prog?
If those resources were acquired earlier in bpf_prog_load(), it looks like
they might be leaked since the put_sdt label is skipped.

> +		}
> +		dst_prog = bpf_prog_get(attr->sdt.target_prog_fd);

[Severity: High]
Could overwriting dst_prog here leak the BPF program reference? If the caller
populated both attr->attach_prog_fd and attr->sdt.target_prog_fd, dst_prog
would already hold a reference that seems to get lost on this assignment.

> +		if (IS_ERR(dst_prog)) {
> +			err = PTR_ERR(dst_prog);
> +			dst_prog = NULL;
> +			goto put_token;
> +		}
[ ... ]
>  	prog->expected_attach_type = attr->expected_attach_type;
>  	prog->sleepable = !!(attr->prog_flags & BPF_F_SLEEPABLE);
>  	prog->aux->attach_btf = attach_btf;
> -	prog->aux->attach_btf_id = multi_func ? bpf_multi_func_btf_id[0] : attr->attach_btf_id;
> +	if (prog_sdt_probe)
> +		prog->aux->attach_btf_id = attach_btf_id;
> +	else
> +		prog->aux->attach_btf_id =
> +			multi_func ? bpf_multi_func_btf_id[0] : attr->attach_btf_id;
>  	prog->aux->dst_prog = dst_prog;
> +	prog->aux->sdt_probe = prog_sdt_probe;
> +	/* ownership of dst_prog/attach_btf/prog_sdt_probe moved to prog->aux */
> +	dst_prog = NULL;
> +	attach_btf = NULL;
> +	prog_sdt_probe = NULL;

[Severity: High]
Will early load failures after this point leak prog->aux->sdt_probe?

On subsequent errors, the code jumps to free_prog which calls bpf_prog_free().
Since bpf_prog_free() and its deferred cleanup routines don't seem to free
sdt_probe (unlike __bpf_prog_put_noref() which is bypassed on early failures),
does this leave the probe info allocated?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782571533.git.xukuohai@huawei.com?part=9