Re: [PATCH 5/5] perf dso: Replace assert with runtime check in dso__read_symbol()

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

Pre-existing issues:
- [Medium] dso__read_symbol() fails to offset into the concatenated BPF JIT instruction buffer for subprograms, unconditionally returning the instructions for the first subprogram.
- [Medium] The `is_64bit` flag for BPF JIT disassembly is hardcoded to the host's `sizeof(void *)` instead of reflecting the target's architecture, breaking cross-platform disassembly.
--

--- Patch [5]: [PATCH 5/5] perf dso: Replace assert with runtime check in dso__read_symbol() ---
commit 53ccbff8c0716057dbbc219812e142e5e96f1fe9
Author: Arnaldo Carvalho de Melo <[email protected]>

    perf dso: Replace assert with runtime check in dso__read_symbol()
    
    dso__read_symbol() asserts that len <= jited_prog_len, where both
    values originate from untrusted file input. With NDEBUG, the assert
    is compiled out, allowing an out-of-bounds heap read. This replaces
    the assert with a runtime bounds check that returns NULL.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index a0de56c93592a..f3209f285db76 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -2024,7 +2024,12 @@ const u8 *dso__read_symbol(struct dso *dso, const char *symfs_filename,

[Severity: Medium]
This is a pre-existing issue in dso__read_symbol(), but should the
is_64bit flag be based on the host's architecture?

Earlier in this function, it sets:

    *is_64bit = sizeof(void *) == sizeof(u64);

If analyzing a perf.data file generated on a 32-bit target using a 64-bit
host, won't this cause cross-platform disassembly to break because downstream
disassemblers receive the wrong bitness flag? Could we dynamically inspect
the target machine architecture from perf_env (e.g. e_machine) instead?

>  			errno = SYMBOL_ANNOTATE_ERRNO__BPF_MISSING_BTF;
>  			return NULL;
>  		}
> -		assert(len <= info_linear->info.jited_prog_len);
> +		if (len > info_linear->info.jited_prog_len) {
> +			pr_debug("BPF symbol length %zu exceeds jited_prog_len %u\n",
> +				 len, info_linear->info.jited_prog_len);
> +			errno = SYMBOL_ANNOTATE_ERRNO__BPF_MISSING_BTF;
> +			return NULL;
> +		}
>  		*out_buf_len = len;
>  		return (const u8 *)(uintptr_t)(info_linear->info.jited_prog_insns);

[Severity: Medium]
This isn't a bug introduced by this patch, but does returning the base of
the JITed instructions handle subprograms correctly?

If the perf.data file contains a multi-function BPF program (where
dso__bpf_prog(dso)->sub_id > 0), this appears to blindly return the
instructions for the first subprogram instead of the requested subprogram.

Should this calculate the byte offset for the target subprogram using
jited_func_lens?

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