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

On 8/6/2026 12:33 PM, [email protected] wrote:
> 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?
> 

Good catch, thanks! I missed the sanity check for duplicate sections.
I will add a check `if (obj->kmod_btfs) return -EEXIST;` to prevent duplicate
section parsing and fix this potential memory leak in v2.

>> +	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.
> 

My apologies for this careless oversight in error handling. Leaving `err` as 0
here indeed creates a dangerous silent error.

I will assign `err = -ENOMEM;` right before jumping to `err_out` and thoroughly
re-check every error path in the patch series for v2. Thanks again

>> +	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.
> 

This is a valid point regarding pointer lifetime.

In v2, I will update `is_kmod_btf_needed()` to store persistent string
pointers from the pre-parsed ELF section instead of the stack buffer.

Thanks for pointing this out.

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