Re: [PATCH 1/4] perf libbfd: Validate BPF prog info arrays before pointer cast

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

Pre-existing issues:
- [High] Missing bitmask validation for several PERF_BPIL_* flags before calling bpf_prog_linfo__new(), leading to raw file offset dereferences.
- [High] Missing bitmask validation for PERF_BPIL_PROG_TAGS and PERF_BPIL_FUNC_INFO in bpf-event.c call sites.
--

--- Patch [1]: [PATCH 1/4] perf libbfd: Validate BPF prog info arrays before pointer cast ---
commit 91de4e21d8f65551a65e6afd999ee44cac7b52f7
Author: Arnaldo Carvalho de Melo <[email protected]>

perf libbfd: Validate BPF prog info arrays before pointer cast

This commit adds bitmask checks for PERF_BPIL_JITED_INSNS and
PERF_BPIL_JITED_KSYMS in libbfd.c before casting them to pointers. This
prevents raw file offsets from being dereferenced when processing a crafted
perf.data file.

> perf libbfd: Validate BPF prog info arrays before pointer cast
>
> symbol__disassemble_bpf_libbfd() casts info_linear->info.jited_prog_insns
> and info_linear->info.jited_ksyms to pointers without checking whether
> bpil_offs_to_addr() actually converted the file offsets.  A crafted
> perf.data with PERF_BPIL_* bits unset but non-zero counts causes raw
> file offsets to be dereferenced as pointers.
>
> Add bitmask checks for PERF_BPIL_JITED_INSNS and PERF_BPIL_JITED_KSYMS
> before the casts, matching the validation added to bpf-event.c call
> sites.

[Severity: High]
This is a pre-existing issue, but is the validation in the bpf-event.c call
sites complete?

In tools/perf/util/bpf-event.c:__bpf_event__print_bpf_prog_info(), the
required_arrays bitmask fails to include PERF_BPIL_PROG_TAGS and
PERF_BPIL_FUNC_INFO.

It then proceeds to call synthesize_bpf_prog_name(), which dereferences
info->prog_tags without checking if they were converted:

tools/perf/util/bpf-event.c:synthesize_bpf_prog_name() {
    ...
    name_len += snprintf_hex(buf + name_len, size - name_len,
                             prog_tags[sub_id], BPF_TAG_SIZE);
    ...
}

Could a crafted perf.data file provide non-zero counts but omit the
corresponding bits in the array mask, leaving them as unconverted raw
file offsets?

> diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
> index c1c12308cc12f..d6c2d9807fc9c 100644
> --- a/tools/perf/util/libbfd.c
> +++ b/tools/perf/util/libbfd.c

[ ... ]

> @@ -581,6 +586,12 @@ int symbol__disassemble_bpf_libbfd(struct symbol *sym __maybe_unused,
>  	if (disassemble == NULL)
>  		abort();
>  
> +	/* jited_ksyms is only valid if bpil_offs_to_addr() converted it */
> +	if (!(info_linear->arrays & (1UL << PERF_BPIL_JITED_KSYMS))) {
> +		ret = SYMBOL_ANNOTATE_ERRNO__BPF_MISSING_BTF;
> +		goto out;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does calling bpf_prog_linfo__new() earlier
in this function before validating the PERF_BPIL_JITED_KSYMS bitmask risk
dereferencing raw file offsets?

Because bpf_prog_linfo__new() relies on jited_ksyms internally, putting the
validation for PERF_BPIL_JITED_KSYMS here means the validation happens after
the structure has already been accessed:

tools/lib/bpf/bpf_prog_linfo.c:dissect_jited_func() {
    ...
    if (ksym_func[0] != *jited_linfo)
    ...
}

It also seems that bpf_prog_linfo__new() uses info->line_info, which doesn't
have a PERF_BPIL_LINE_INFO check before the call.

Should the validation for PERF_BPIL_JITED_KSYMS (and other arrays like
line_info) be moved earlier, before info_linear->info is passed to
bpf_prog_linfo__new()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1