Re: [PATCH bpf-next 11/13] bpf: keep module BTF until the vmlinux BTF is parsed

[sashiko-bot@kernel.org]

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 <wanjay@amazon.com>

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/20261031130959.23472-1-wanjay@amazon.com?part=11