Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] bpf: NULL deref in btf_parse_kept_modules() after inline sysfs failure - [High] bpf: UAF of btf_mod->data in btf_free_rcu() after kept module unparse - [High] bpf: premature btf_idr publication in btf_parse_kept_modules() causes UAF - [Medium] bpf: btf_parse_kept_modules() drops kept inline BTF on -ENOMEM - [Medium] bpf: drop kept module BTFs in btf_drop_all_deferred() on vmlinux BTF failure --
commit c77c66110b385c53a4bc18e94bea01ef20eb7416 Author: Jay Wang <[email protected]> bpf: keep module BTF until the vmlinux BTF is parsed With CONFIG_DEBUG_INFO_BTF_LAZY enabled, this commit keeps in-tree module BTF and inline BTF unparsed when a module loads before vmlinux BTF is parsed, exposing their sysfs files immediately and parsing the kept module BTFs under btf_vmlinux_lock when vmlinux BTF is first parsed. > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index a5b1cd63a1e43..631f5a32ec174 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c [ ... ] > @@ -9395,6 +9398,149 @@ static DEFINE_MUTEX(btf_module_mutex); > static void purge_cand_cache(struct btf *btf); > static void btf_drop_deferred_regs(struct module *owner); > > +/* /sys/kernel/btf/<module> and <module>.inline */ > +static void btf_module_add_files(struct btf_module *btf_mod, const char > *name, > + void *data, u32 data_size) > +{ > + char inline_name[MODULE_NAME_LEN + sizeof(".inline")]; > + struct bin_attribute *attr; > + > + attr = sysfs_btf_add(name, data, data_size, false, NULL); > + if (IS_ERR(attr)) > + return; > + btf_mod->sysfs_attr = attr; > + > + if (!btf_mod->btf_inline_data) > + return; > + snprintf(inline_name, sizeof(inline_name), "%s.inline", name); > + attr = sysfs_btf_add(inline_name, btf_mod->btf_inline_data, > + btf_mod->btf_inline_data_size, false, NULL); > + if (IS_ERR(attr)) { > + kvfree(btf_mod->btf_inline_data); > + btf_mod->btf_inline_data = NULL; > + return; > + } [Severity: High] Can this trigger a NULL pointer dereference when btf_parse_kept_modules() later parses this module? In btf_module_keep(), btf_mod->btf_inline_data_size is set to a non-zero inline_data_size before calling btf_module_add_files(). If sysfs_btf_add() fails for the .inline file here, btf_mod->btf_inline_data is freed and set to NULL, but btf_mod->btf_inline_data_size remains non-zero. Later, btf_parse_kept_modules() passes btf_mod->btf_inline_data (NULL) and btf_mod->btf_inline_data_size (non-zero) to btf_parse_module(): if (inline_data_size) { inline_btf = btf_parse_split(env, module_name, inline_data, inline_data_size, btf); which calls btf_parse_split() -> btf_parse_hdr() with btf->data == NULL and btf->data_size > 0, dereferencing hdr = btf->data. Should btf_mod->btf_inline_data_size also be reset to 0 here (and if the first sysfs_btf_add() call fails above, should btf_mod->btf_inline_data also be freed and cleared)? > + btf_mod->sysfs_inline_attr = attr; > +} [ ... ] > +/* Undo btf_parse_kept_modules() of a round that is not published */ > +static void btf_unparse_kept_modules(void) > +{ > + struct btf_module *btf_mod; > + > + lockdep_assert_held(&btf_module_mutex); > + list_for_each_entry(btf_mod, &btf_modules, list) { > + if (!btf_mod->data || !btf_mod->btf) > + continue; > + btf_mod->btf->data = NULL; /* the kept copy */ > + btf_put(btf_mod->btf); > + btf_mod->btf = NULL; > + } > +} [Severity: High] Could btf_free_rcu() read freed btf_mod->data memory after btf_unparse_kept_modules() runs? In btf_apply_deferred(), after btf_parse_kept_modules() sets up btf_mod->btf in place over btf_mod->data, an earlier deferred BTF_DEFER_ST_OPS registration can populate btf_mod->btf->struct_ops_tab via __register_bpf_struct_ops() -> btf_add_struct_ops() -> bpf_struct_ops_desc_init(), storing st_ops_desc->type = btf_type_by_id() which points directly into btf_mod->data. If a subsequent deferred registration in btf_apply_deferred() fails with -ENOMEM, btf_unparse_kept_modules() sets btf_mod->btf->data = NULL, schedules asynchronous destruction of btf_mod->btf via btf_put() -> call_rcu(), and resets btf_mod->btf = NULL. If the module unloads (MODULE_STATE_GOING) before that RCU grace period finishes, btf_module_notify() sees !btf_mod->btf and calls btf_module_drop_kept(btf_mod), which immediately frees btf_mod->data with kvfree(). When btf_free_rcu() -> btf_free() -> btf_free_struct_ops_tab() -> bpf_struct_ops_desc_release() later executes: arg_info = st_ops_desc->arg_info; for (i = 0; i < btf_type_vlen(st_ops_desc->type); i++) kfree(arg_info[i].info); doesn't btf_type_vlen(st_ops_desc->type) dereference the already freed btf_mod->data buffer? > + > +/* > + * Called under btf_vmlinux_lock by the task that parses the vmlinux BTF, > + * before that is published. Parses the kept module BTFs in place, oldest > + * first so that ids follow the load order as without > + * CONFIG_DEBUG_INFO_BTF_LAZY; in-tree module BTF and its inline BTF need no > + * relocation, so sysfs serves the same bytes before and after. A module > + * whose BTF fails validation cannot be refused any more: it stays loaded > + * without BTF, and its files go away; an invalid inline BTF loses its file. > + * Returns -ENOMEM, with this round's module BTFs freed again, if memory ran > + * out. > + */ > +static int btf_parse_kept_modules(void) > +{ > + struct btf_module *btf_mod, *tmp; > + void *inline_data; > + struct btf *btf; > + int err; > + > + lockdep_assert_held(&btf_vmlinux_lock); > + guard(mutex)(&btf_module_mutex); > + list_for_each_entry_safe_reverse(btf_mod, tmp, &btf_modules, list) { > + if (btf_mod->btf || !btf_mod->data) > + continue; > + btf = btf_parse_module(btf_mod->module->name, btf_mod->data, > + btf_mod->data_size, NULL, 0, > + btf_mod->btf_inline_data, > + btf_mod->btf_inline_data_size, > &inline_data); > + if (!IS_ERR(btf)) { > + err = btf_alloc_id(btf); > + if (err) { > + btf->data = NULL; /* the kept copy */ > + btf_free(btf); > + btf = ERR_PTR(err); > + } > + } [Severity: High] Is it safe to publish btf into btf_idr via btf_alloc_id(btf) before the rest of btf_parse_kept_modules() and btf_apply_deferred() have succeeded? In btf_get_fd_by_id(), the return value of bpf_get_btf_vmlinux() is ignored: if (IS_ENABLED(CONFIG_DEBUG_INFO_BTF_LAZY)) bpf_get_btf_vmlinux(); rcu_read_lock(); btf = idr_find(&btf_idr, id); if (!btf || !refcount_inc_not_zero(&btf->refcnt)) If task A's bpf_get_btf_vmlinux() fails with -ENOMEM, task A still proceeds to idr_find(&btf_idr, id) while task B is inside btf_apply_deferred() -> btf_parse_kept_modules(). Task A (or a deferred struct_ops ->init() callback calling bpf_find_btf_id()) can grab a reference to btf_mod->btf from btf_idr, raising refcnt above 1. If task B then hits -ENOMEM on a later module or deferred registration, btf_unparse_kept_modules() sets btf_mod->btf->data = NULL and calls btf_put(btf_mod->btf), and bpf_get_btf_vmlinux() frees the unpublished vmlinux BTF via btf_put(). Because btf_mod->btf->refcnt > 1, btf_put() does not free btf_mod->btf or remove it from btf_idr, leaving a live BTF object with btf->data == NULL (which crashes in btf_get_info_by_fd() on copy_to_user(ubtf, btf->data, btf_copy)) and a dangling btf->base_btf pointer to the freed vmlinux BTF. > + if (btf == ERR_PTR(-ENOMEM)) { > + btf_unparse_kept_modules(); > + return -ENOMEM; > + } > + if (IS_ERR(btf)) { > + pr_warn("failed to validate module [%s] BTF: %ld\n", > + btf_mod->module->name, PTR_ERR(btf)); > + btf_module_drop_kept(btf_mod); > + continue; > + } > + btf_mod->btf = btf; > + if (btf_mod->btf_inline_data && !inline_data) { > + if (btf_mod->sysfs_inline_attr) > + sysfs_btf_remove(btf_mod->sysfs_inline_attr); > + btf_mod->sysfs_inline_attr = NULL; > + kvfree(btf_mod->btf_inline_data); > + btf_mod->btf_inline_data = NULL; > + btf_mod->btf_inline_data_size = 0; > + } [Severity: Medium] If parsing a kept module's inline BTF fails with -ENOMEM inside btf_parse_module(), doesn't this permanently drop the module's inline BTF and sysfs file instead of rolling back the round? In btf_parse_module(), an error from btf_parse_split() on inline_btf (such as ERR_PTR(-ENOMEM)) is logged and cleared to inline_btf = NULL, returning the main module btf with inline_data == NULL. btf_parse_kept_modules() then treats !inline_data as a validation failure and removes btf_mod->sysfs_inline_attr and frees btf_mod->btf_inline_data, even if the round fails with -ENOMEM and is retried later. > + } > + return 0; > +} [ ... ] > @@ -11206,8 +11349,13 @@ static int btf_apply_deferred(void) > break; > } > module_put(reg->owner); > - if (err == -ENOMEM) > + if (err == -ENOMEM) { > +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES > + guard(mutex)(&btf_module_mutex); > + btf_unparse_kept_modules(); > +#endif > return err; > + } > if (err) > pr_warn("failed to apply deferred BTF registration for > %s: %d\n", > reg->owner ? reg->owner->name : "vmlinux", err); [Severity: Medium] What happens to kept module BTFs if btf_parse_vmlinux() in bpf_get_btf_vmlinux() fails with a non-ENOMEM error (or btf_apply_deferred() were to return a non-ENOMEM error)? In bpf_get_btf_vmlinux(), when btf is a non-ENOMEM error pointer, btf_apply_deferred() is not run (or fails) and btf_drop_all_deferred() is called before publishing the error pointer to btf_vmlinux. However, btf_drop_all_deferred() only frees entries in btf_deferred_regs and does not call btf_module_drop_kept() on kept entries in btf_modules, leaving their unparsed data buffers and /sys/kernel/btf/<module> sysfs files around until module unload. Should btf_drop_all_deferred() also drop kept module entries? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
