Re: [RFC PATCH bpf-next 05/12] bpf: Collect SDT probe BTF IDs from BTF decl tags

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Missing bounds check for `val.nargs` during `BPF_MAP_TYPE_INSN_ARRAY` map update allows a massive out-of-bounds stack write (buffer overflow).
- [High] Modifying the size of `struct bpf_insn_array_value` breaks ABI compatibility.
- [Medium] `sdt_collect_btf_ids()` unconditionally dereferences `obj->btf`, causing a segmentation fault when an ELF file lacks BTF info.
- [Medium] The `<nargs>` parameter in the `bpf_sdt:<name>:<nargs>` BTF tag is ignored during parsing, potentially causing incorrect BTF ID assignments.
--

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

bpf: Collect SDT probe BTF IDs from BTF decl tags

Collect SDT probe BTF IDs from BTF decl tags matching name pattern
"bpf_sdt:<name>:<nargs>". The IDs are ultimately stored in each probe
entry of the SDT map.

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7739,6 +7739,7 @@ struct bpf_insn_array_value {
>  	__u32 orig_off;
>  	__u32 xlated_off;
>  	__u32 jitted_off;
> +	__u32 btf_id;		/* BTF FUNC_PROTO type ID (0 if none) */

[Severity: High]
Does changing the size of struct bpf_insn_array_value break ABI backward
compatibility?

When libbpf attempts to load a BPF program on an older kernel, create_jt_map()
will use the new sizeof(struct bpf_insn_array_value) for the .jumptables map
value size.

Will older kernels reject this map creation since they expect the value size
to be exactly the old size?

>  	__u8  nargs;		/* argument count (0..5) */
>  	__u8  arg_reg[5];	/* BPF register for each argument */
>  	__u8  pad[2];

[ ... ]

> diff --git a/kernel/bpf/bpf_insn_array.c b/kernel/bpf/bpf_insn_array.c
> --- a/kernel/bpf/bpf_insn_array.c
> +++ b/kernel/bpf/bpf_insn_array.c
> @@ -103,6 +103,7 @@ static long insn_array_update_elem(struct bpf_map *map, void *key, void *value,
>  	insn_array->values[index].orig_off = val.orig_off;
>  
>  	if (insn_array->subtype == BPF_INSN_ARRAY_SUBTYPE_SDT) {
> +		insn_array->values[index].btf_id = val.btf_id;
>  		insn_array->values[index].nargs = val.nargs;

[Severity: Critical]
Is there a bounds check missing for val.nargs before it is copied here?

If a malicious user provides a large value for val.nargs during map update,
could this cause an out-of-bounds write later in the attach path?

For example, in bpf_sdt_link_attach(), the loop iterates based on this value:

bpf_sdt_link_attach() {
    ...
    tgt_info.fmodel.nr_args = sdt->val.nargs;
    for (i = 0; i < sdt->val.nargs; i++) {
        tgt_info.fmodel.arg_size[i] = 8;
        tgt_info.fmodel.arg_regs[i] = sdt->val.arg_reg[i];
    }
    ...
}

Since the target arrays have a fixed capacity, would a large nargs value
overflow the kernel stack buffer?

>  		memcpy(insn_array->values[index].arg_reg, val.arg_reg, sizeof(val.arg_reg));
>  	}

[ ... ]

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -6692,6 +6693,57 @@ static int create_jt_map(struct bpf_object *obj, struct bpf_program *prog, struc
>  	return err;
>  }
>  
> +static void sdt_collect_btf_ids(struct bpf_object *obj)
> +{
> +	int n = btf__type_cnt(obj->btf);

[Severity: Medium]
Can obj->btf be NULL here if the ELF object was compiled without BTF
information?

If so, would calling btf__type_cnt() on a NULL pointer cause a segmentation
fault during bpf_object__open() when parsing SDT notes?

> +	const struct btf_type *t;
> +	int i;

[ ... ]

> +		/* match by name (tag format: "bpf_sdt:<name>:<nargs>") */
> +		for (j = 0; j < obj->sdt_entry_cnt; j++) {
> +			struct sdt_entry *e = &obj->sdt_entries[j];
> +
> +			if (!e->btf_id && strlen(e->name) == name_len &&
> +			    strncmp(tag + 8, e->name, name_len) == 0)
> +				e->btf_id = func_proto_id;

[Severity: Medium]
Does this string comparison logic correctly handle overloaded SDT probes?

The code parses the tag format "bpf_sdt:<name>:<nargs>" and compares the
probe name, but it appears to ignore the nargs substring when matching
the tag.

If multiple probes share the same name but have different argument counts,
could this assign the incorrect BTF ID to a probe, causing the kernel to fail
the program load when verifying the argument count?

> +		}
> +	}
> +}

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