Re: [RFC PATCH bpf-next 07/12] bpf: Record probe name in SDT map

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Strict `value_size` validation on the expanded `struct bpf_insn_array_value` breaks backwards and forwards compatibility.
- [Medium] Memory leak of `e->name` on the error path in `bpf_object__collect_sdt_notes` when a probe name exceeds `BPF_SDT_MAX_NAME_LEN`.
--

commit 33292e7627dcc830a8b1dc17788ae56ec52849c6
Author: Xu Kuohai <xukuohai@huawei.com>

bpf: Record probe name in SDT map

This commit adds a probe name field to the bpf_insn_array_value structure to
help the kernel locate target programs and probe sites. It enforces a maximum
name length of 64 characters and updates libbpf to pass the name during map
creation.

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 773a7def0fbd3..95ca41bf75018 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
[ ... ]
> @@ -7743,6 +7744,7 @@ struct bpf_insn_array_value {
>  	__u8  nargs;		/* argument count (0..5) */
>  	__u8  arg_reg[5];	/* BPF register for each argument */
>  	__u8  pad[2];
> +	char  name[BPF_SDT_MAX_NAME_LEN]; /* probe name */
>  };

[Severity: Critical]
Does changing the size of struct bpf_insn_array_value break backwards
compatibility with older userspace applications?

It appears the kernel enforces a strict value_size check during map creation
in insn_array_alloc_check():

	if (attr->max_entries == 0 || attr->key_size != 4 ||
	    attr->value_size != value_size ||
	    attr->map_flags & ~BPF_F_INSN_ARRAY_SDT)
		return -EINVAL;

Because value_size is derived from sizeof(struct bpf_insn_array_value),
older binaries passing a smaller value_size of 16 to
bpf_map_create(BPF_MAP_TYPE_INSN_ARRAY) might now be rejected with -EINVAL.

[ ... ]
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index badac128e8adc..dbc08a1931015 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4146,6 +4146,12 @@ static int bpf_object__collect_sdt_notes(struct bpf_object *obj)
>  			err = -ENOMEM;
>  			goto out;
>  		}
> +		if (strlen(e->name) >= BPF_SDT_MAX_NAME_LEN) {
> +			pr_warn("sdt: probe name '%s' too long (max %d chars)\n",
> +				e->name, BPF_SDT_MAX_NAME_LEN - 1);
> +			err = -EINVAL;
> +			goto out;
> +		}

[Severity: Medium]
Can this goto leak the newly allocated e->name string?

Just above this snippet, e->name is allocated:

		e->name = strdup(sdt_syms[i].name + 10);
		if (!e->name) {
			err = -ENOMEM;
			goto out;
		}

If the length exceeds BPF_SDT_MAX_NAME_LEN, we jump to the out label.
However, the cleanup loop in out:

out:
	if (err) {
		for (i = 0; i < obj->sdt_entry_cnt; i++)
			free(obj->sdt_entries[i].name);

only frees up to obj->sdt_entry_cnt, which hasn't been incremented yet for
this entry, potentially causing the string to be leaked.

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