Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Kernel panic via unconditional btf_free(NULL) on error path - [Medium] Memory leak of vmlinux.inline sysfs attribute data across module unloads and reloads - [Medium] Concurrent updates and reads of `vmlinux.inline` BTF sysfs data can cause an out-of-bounds read --
commit aad19c968ab1d821749bb6d06563d7c786f0721d Author: Alan Maguire <[email protected]> btf: Relocate inline BTF for modules with distilled base BTF This commit adds support for parsing, relocating, and publishing inline BTF for modules that use a distilled base BTF. It parses the module's inline BTF relative to the original module BTF, rewrites the IDs and string offsets, and ensures a failure to process inline BTF doesn't block the regular module BTF registration. > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 2ac1f1d396608..2f3e8cea7dfcd 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c [ ... ] > @@ -6727,15 +6727,140 @@ __u32 btf_relocate_id(const struct btf *btf, __u32 > id) [ ... ] > errout: > + kvfree(module_str_map); > btf_verifier_env_free(env); > + btf_free(inline_btf); > if (!IS_ERR(base_btf) && base_btf != vmlinux_btf) > btf_free(base_btf); > - if (btf) { > - kvfree(btf->data); > - kvfree(btf->types); > - kfree(btf); > - } > + btf_free(btf); [Severity: High] Is it safe to unconditionally call btf_free() on these pointers here? If btf_parse_split() returns an error, btf or inline_btf can be NULL before jumping to errout. Looking at btf_free(), it immediately passes the pointer to btf_free_struct_meta_tab(), which dereferences it without checking for NULL: static void btf_free(struct btf *btf) { btf_free_struct_meta_tab(btf); ... static void btf_free_struct_meta_tab(struct btf *btf) { struct btf_struct_metas *tab = btf->struct_meta_tab; Could this lead to a NULL pointer dereference and a kernel panic during an error path? > return ERR_PTR(err); > } [ ... ] > @@ -8958,32 +9104,26 @@ static int btf_module_notify(struct notifier_block > *nb, unsigned long op, > btf_mod->sysfs_attr = attr; > > #if IS_ENABLED(CONFIG_DEBUG_INFO_BTF_INLINE) > - if (mod->btf_inline_data_size > 0) { > + if (relocated_inline_data) { > char name[MODULE_NAME_LEN + sizeof(".inline")]; > > if (strcmp(mod->name, "btf_vmlinux_inline") == 0) { > if (vmlinux_inline_attr) > - sysfs_btf_update(vmlinux_inline_attr, > data, > - > mod->btf_inline_data_size); > + sysfs_btf_update(vmlinux_inline_attr, > + relocated_inline_data, > inline_data_size); [Severity: Medium] Could the concurrent update of this sysfs attribute cause an out-of-bounds read? Looking at sysfs_btf_update(), the new pointer is written before the new size: static void sysfs_btf_update(struct bin_attribute *attr, void *data, size_t data_size) { ... WRITE_ONCE(attr->private, data); smp_store_release(&attr->size, data_size); } If a concurrent reader in sysfs_btf_kernfs_read() picks up the old, larger size via smp_load_acquire(), but then reads the newly updated pointer from attr->private via READ_ONCE(), wouldn't the subsequent memcpy() read out-of-bounds past the end of the new, smaller data buffer? > else > - kvfree(data); > + kvfree(relocated_inline_data); > + btf_mod->btf_inline_data = NULL; [Severity: Medium] Does setting btf_mod->btf_inline_data to NULL here cause a memory leak when the btf_vmlinux_inline module is unloaded? Because it's set to NULL, the normal cleanup path in btf_module_notify() won't free it. Then, if the module is reloaded, sysfs_btf_update() will overwrite the vmlinux_inline_attr->private pointer with a new allocation, leaving the old relocated_inline_data stranded. > break; > } [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
