Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] bpf: UAF in sysfs reader on module BTF alloc ID failure - [High] bpf: data corruption via un-synchronized kfunc registrations - [Medium] bpf: race condition causes transient BPF program load failures - [Medium] bpf: memory leak of vmlinux deferred queue on alloc ID failure --
commit 7468b94a7c4356b1f1498b95a9030eec1830ffa1 Author: Jay Wang <[email protected]> Subject: bpf: defer registrations until the vmlinux BTF is available This commit changes the BPF subsystem to defer kfunc, dtor kfunc, and struct_ops registrations until the vmlinux BTF is parsed. This allows configuring CONFIG_DEBUG_INFO_BTF as a module so it doesn't need to be parsed at boot if not required. > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 50eb7a95fd825..cbba20a908e9d 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c [ ... ] > @@ -6581,7 +6583,15 @@ struct btf *btf_parse_vmlinux(void) > if (err) { > btf_free(btf); > btf = ERR_PTR(err); > + goto err_out; > } > + > + /* > + * With CONFIG_DEBUG_INFO_BTF=m, kfunc, dtor kfunc and struct_ops > + * registrations for vmlinux made before the BTF was available were > + * queued; apply them now, before the BTF becomes visible to anyone. > + */ > + btf_apply_deferred_vmlinux_regs(btf); > err_out: > btf_verifier_env_free(env); > return btf; [Severity: Medium] When btf_alloc_id() fails, the code jumps to err_out, which bypasses the call to btf_apply_deferred_vmlinux_regs(). Does this leave the items in the btf_vmlinux_deferred_regs queue stranded, causing a memory leak for those dynamically allocated deferred registrations? Also, since btf_vmlinux_regs_closed is never set, would subsequent registrations continue to queue infinitely? [ ... ] > @@ -8905,6 +9033,87 @@ static int __init btf_module_init(void) > } > > fs_initcall(btf_module_init); > + > +#if IS_MODULE(CONFIG_DEBUG_INFO_BTF) > +/* > + * CONFIG_DEBUG_INFO_BTF=m: the vmlinux BTF has just become available. Parse > + * the BTF of the modules that were loaded before it, and apply the > + * registrations that waited for them. Called from bpf_get_btf_vmlinux() > + * once btf_vmlinux is published, with no locks held. > + */ > +void btf_parse_deferred_modules(void) > +{ [ ... ] > + err = PTR_ERR_OR_ZERO(btf); > + if (!err) { > + err = btf_alloc_id(btf); > + if (err) { > + /* btf owns the data now, btf_free() drops it */ > + btf_mod->data = NULL; > + btf_free(btf); > + } > + } > + if (err) { > + /* > + * The module is loaded and stays. Unlike at load time > + * there is no way to reject it, so drop its BTF. > + */ > + pr_warn("failed to validate module [%s] BTF: %d\n", > + btf_mod->module->name, err); > + list_del(&btf_mod->list); > + btf_module_free(btf_mod); > + continue; > + } [Severity: High] Could the btf_free() call above introduce a use-after-free window for concurrent sysfs readers? The backing buffer btf->data (which was originally btf_mod->data) is freed by btf_free(). Even though btf_mod->data is set to NULL, the sysfs file for the module remains active until btf_module_free() removes it. If there is a concurrent read to /sys/kernel/btf/<module>, can it dereference the freed attr->private pointer? [ ... ] > + /* btf->data is btf_mod->data now, the sysfs file keeps working > */ > + btf_mod->data = NULL; > + kvfree(btf_mod->base_data); > + btf_mod->base_data = NULL; > + btf_mod->btf = btf; > + parsed = true; [Severity: Medium] Here btf_mod->btf is assigned, which makes the module BTF visible globally. Since this happens before the deferred kfuncs and struct_ops are actually registered, is there a race condition where a concurrent BPF program load attempts to use this BTF, but fails verification because the kfuncs are not yet in the module's kfunc_set_tab? > + > + /* > + * Registrations are applied after dropping the mutex (they > + * walk btf_modules); pin what they need until then. > + */ > + list_for_each_entry_safe(reg, rtmp, &btf_mod->deferred_regs, > list) { > + list_del(®->list); > + if (!try_module_get(btf_mod->module)) { > + btf_free_deferred_reg(reg); > + continue; > + } > + btf_get(btf); > + reg->btf = btf; > + reg->module = btf_mod->module; > + list_add_tail(®->list, ®s); > + } > + } > + mutex_unlock(&btf_module_mutex); > + > + if (parsed) > + purge_cand_cache(NULL); > + btf_apply_deferred_regs(®s); > +} > +#endif /* IS_MODULE(CONFIG_DEBUG_INFO_BTF) */ [Severity: High] Is it possible for data corruption to occur here due to concurrent kfunc registrations? Because the btf_module_mutex is dropped and btf_mod->btf has already been set, a concurrent module initcall could perform a new register_btf_kfunc_id_set(). Since btf_mod->btf is no longer NULL, it would bypass the deferral queue and proceed to btf_kfunc_id_set_add() directly. If btf_apply_deferred_regs() is running simultaneously, both paths could concurrently invoke btf_populate_kfunc_set(), which mutates btf->kfunc_set_tab without synchronization: kernel/bpf/btf.c:btf_populate_kfunc_set() { ... /* Grow set */ set = krealloc(tab->sets[hook], struct_size(set, pairs, set_cnt + add_set->cnt), GFP_KERNEL | __GFP_NOWARN); if (!set) { ret = -ENOMEM; goto end; } /* For newly allocated set, initialize set->cnt to 0 */ if (!tab->sets[hook]) set->cnt = 0; tab->sets[hook] = set; ... } Would this result in double-free or use-after-free on the krealloc, or lost kfunc registrations? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
