Re: [PATCH] libbpf: fix double-free of distilled base BTF on .BTF.ext parse error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] btf_parse_elf() incorrectly uses IS_ERR() to check the return value of the public API btf_ext__new(), rendering the error path and the patched double-free fix unreachable.
- [High] In `btf_parse_elf`, the out-parameter `btf_ext` is not initialized to `NULL` at the beginning of the function. If the function fails early, the error path reads the uninitialized pointer from the caller and passes it to `btf_ext__free()`, causing a crash.
--

--- Patch [1]: [PATCH] libbpf: fix double-free of distilled base BTF on .BTF.ext parse error ---
commit 343ec621fb0622b822c4fef99c23ee0947343232
Author: Naveed Khan <naveed@digiscrypt.com>
Subject: libbpf: fix double-free of distilled base BTF on .BTF.ext parse error

This patch fixes a double-free in the error path of btf_parse_elf(). By
deferring the ownership transfer of the distilled base BTF until after
.BTF.ext is parsed successfully, it ensures the base BTF is freed exactly
once on failure.

> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 823bce895178d..35695f33c6942 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
[ ... ]
> @@ -1530,6 +1527,9 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
>  	} else if (btf_ext) {

[Severity: High]
This isn't a bug introduced by this patch, but immediately before this
else-if block, btf_ext__new() is called and checked using IS_ERR():

	*btf_ext = btf_ext__new(secs.btf_ext_data->d_buf, secs.btf_ext_data->d_size);
	if (IS_ERR(*btf_ext)) {
		err = PTR_ERR(*btf_ext);
		goto done;
	}

Since btf_ext__new() is a public libbpf API, doesn't it return NULL on
error rather than an ERR_PTR?

If IS_ERR(NULL) is false, the error path is never taken for malformed
.BTF.ext sections. Does this effectively make the double-free issue
that this patch fixes structurally unreachable?

>  		*btf_ext = NULL;
>  	}
> +
> +	if (dist_base_btf)
> +		btf->owns_base = true;
>  done:
>  	if (elf)
>  		elf_end(elf);

[Severity: High]
This is a pre-existing issue, but I have a question about the cleanup
path below this done label:

	if (btf_ext)
		btf_ext__free(*btf_ext);

If a caller provides an uninitialized pointer (e.g., struct btf_ext *ext;
btf__parse_elf(path, &ext);) and btf_parse_elf() fails early,
isn't the out-parameter *btf_ext left uninitialized?

Could this cause btf_ext__free() to read uninitialized memory from the
caller's stack and crash? Should *btf_ext be initialized to NULL at the
beginning of btf_parse_elf()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178345549172.94179.7948304165383170781@digiscrypt.com?part=1