Re: [PATCH v2 bpf-next 16/18] btf: Relocate inline BTF for modules with distilled base BTF

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 [off-list ref]

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.

[ ... ]

[ ... ]

 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);
 }

[ ... ]

[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/20260901165757.801449-1-alan.maguire@oracle.com?part=16