Re: [PATCH v3] libbpf: initialize btf_ext out-parameter early in btf_parse_elf()

[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 for btf_ext__new() failure, causing .BTF.ext parsing errors to be silently ignored.
- [High] A latent double-free of dist_base_btf exists on the error path of btf_parse_elf().
--

--- Patch [1]: [PATCH v3] libbpf: initialize btf_ext out-parameter early in btf_parse_elf() ---
commit 5cc9e33bf25334c39330a03f9b8133245f9ed0c1
Author: Naveed Khan <naveed@digiscrypt.com>

libbpf: initialize btf_ext out-parameter early in btf_parse_elf()

This commit initializes the btf_ext out-parameter to NULL early in
btf_parse_elf(). This prevents callers that pass an uninitialized pointer
from encountering stack garbage during cleanup if the function fails before
reaching the .BTF.ext parsing step.

> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 823bce895178d..cabe8286f06c9 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -1453,6 +1453,9 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
>  	int err = 0, fd = -1;
>  	Elf *elf = NULL;
>  
> +	if (btf_ext)
> +		*btf_ext = NULL;
> +
>  	if (elf_version(EV_CURRENT) == EV_NONE) {
>  		pr_warn("failed to init libelf for %s\n", path);
>  		return ERR_PTR(-LIBBPF_ERRNO__LIBELF);

[Severity: High]
This is a pre-existing issue, but does btf_parse_elf() incorrectly use
IS_ERR() to check for btf_ext__new() failure further down in this function?

tools/lib/bpf/btf.c:btf_parse_elf() {
        ...
        if (btf_ext && secs.btf_ext_data) {
                *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;
                }
        ...
}

If btf_ext__new() returns NULL on error rather than ERR_PTR, IS_ERR() will
evaluate to false. This would cause the function to silently ignore
malformed .BTF.ext data or allocation failures, leaving *btf_ext as NULL
and misleading the caller into assuming there is no extended BTF data.

[Severity: High]
This is also a pre-existing issue, but is there a latent double-free of
dist_base_btf on the error path of btf_parse_elf()?

Earlier in the function, ownership of dist_base_btf is transferred to btf:

tools/lib/bpf/btf.c:btf_parse_elf() {
        ...
        if (dist_base_btf)
                btf->owns_base = true;
        ...
}

Then on the error path, both are explicitly freed:

done:
        ...
        btf__free(dist_base_btf);
        btf__free(btf);

If btf->owns_base is true, btf__free(btf) recursively frees its base BTF,
which points to the same dist_base_btf object. If an error occurs after
the ownership transfer (such as if the btf_ext__new() error check is fixed),
would this lead to a double-free?

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