Re: [RFC PATCH bpf-next 1/2] libbpf: support selective kernel module BTF loading via .kmod_btfs section

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Transient stack pointer overwrites persistent hashmap key in is_kmod_btf_needed(), breaking hashmap integrity on bucket collisions.
- [Medium] Memory allocation failure for `obj->kmod_btfs->data` silently returns success (0), masking the error and corrupting intended behavior.
- [Medium] Processing multiple `.kmod_btfs` sections overwrites previous allocations without freeing them.
--

--- Patch [1]: [RFC PATCH bpf-next 1/2] libbpf: support selective kernel module BTF loading via .kmod_btfs section ---
commit 5306ac18160e3a8c2a0f3dd2eda202169f998df6
Author: Fuyu Zhao <[email protected]>

libbpf: support selective kernel module BTF loading via .kmod_btfs section

Add support for a new ELF section ".kmod_btfs" that allows BPF programs
to declare which kernel modules need BTF loading. This avoids loading
all module BTFs and speeds up program load when only a subset of modules
is needed.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 514e4e9daa825..5cefe82e4a679 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -901,6 +916,68 @@ bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog,
>  	return -ENOMEM;
>  }
>  
> +static size_t mod_name_hash_fn(long key, void *ctx)
> +{
> +	return str_hash((char *)key);
> +}
> +
> +static bool mod_name_equal_fn(long key1, long key2, void *ctx)
> +{
> +	return strcmp((char *)key1, (char *)key2) == 0;
> +}
> +
> +static int
> +bpf_object__collect_kmod_btf_names(struct bpf_object *obj, Elf_Data *sec_data,
> +				   const char *sec_name)
> +{
> +	int module_cnt, i, err = 0;
> +
> +	if (sec_data->d_size % KMOD_NAME_LEN != 0) {
> +		pr_warn("sec '%s': size %zu should be multiple of %d\n",
> +			sec_name, sec_data->d_size, KMOD_NAME_LEN);
> +		return -EINVAL;
> +	}
> +
> +	module_cnt = sec_data->d_size / KMOD_NAME_LEN;
> +	obj->kmod_btfs = calloc(1, sizeof(*obj->kmod_btfs));

[Severity: Medium]
If a crafted ELF file contains multiple .kmod_btfs sections, could this calloc
overwrite a previously allocated obj->kmod_btfs and cause a memory leak of
the hashmap and data arrays?

> +	if (!obj->kmod_btfs)
> +		return -ENOMEM;
> +
> +	obj->kmod_btfs->data = calloc(module_cnt, KMOD_NAME_LEN);
> +	if (!obj->kmod_btfs->data)
> +		goto err_out;

[Severity: Medium]
Does this error path silently return success?

Because err is initialized to 0, this goto err_out will return 0 instead of
-ENOMEM. This causes obj->kmod_btfs to be freed in the cleanup path while
the function signals success, making load_module_btfs() fall back to loading
all module BTFs.

> +	memcpy(obj->kmod_btfs->data, sec_data->d_buf, sec_data->d_size);

[ ... ]

> @@ -5803,6 +5884,21 @@ int bpf_core_add_cands(struct bpf_core_cand *local_cand,
>  	return 0;
>  }
>  
> +static bool is_kmod_btf_needed(struct bpf_object *obj, const char *name)
> +{
> +	uintptr_t val;
> +
> +	if (!hashmap__find(obj->kmod_btfs->hashmap, name, &val))
> +		return false;
> +
> +	if (val == KMODS_BTF_LOADED)
> +		return false;
> +
> +	hashmap__set(obj->kmod_btfs->hashmap, name, KMODS_BTF_LOADED, NULL, NULL);

[Severity: Medium]
Can this hashmap__set() corrupt the hashmap by storing a transient stack
pointer as the key?

Looking at load_module_btfs(), name is a local stack array passed to
is_kmod_btf_needed() on every iteration:

load_module_btfs() {
    char name[64];
    ...
        if (obj->kmod_btfs && obj->kmod_btfs->hashmap &&
            !is_kmod_btf_needed(obj, name)) {
    ...
}

Since hashmap__set() only stores the pointer address, all entries will
share the same pointer to the local name array. If a bucket collision
occurs, strcmp(key1, key2) will evaluate to 0 since both point to the
same stack address, falsely matching existing entries and skipping the
requested module.

> +	obj->kmod_btfs->nr_loaded++;
> +	return true;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1