Re: [PATCH v5 05/26] perf annotate: Normalize arch__dwarf_regnum() error return values

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The check_matching_type() function uses dloc->op->reg1 as an array index without bounds checking, leading to an out-of-bounds stack access when a register cannot be parsed.
--

commit 7d09889746335253aa4ac524e693986afc798032
Author: Tengda Wu <wutengda@huaweicloud.com>

perf annotate: Normalize arch__dwarf_regnum() error return values

This commit normalizes the error return values of arch__dwarf_regnum() so
that all failures consistently return -1. This ensures callers that only
check for -1 will correctly identify genuine failures.

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -2492,7 +2492,7 @@ static int arch__dwarf_regnum(const struct arch *arch, const char *str)
>  
>  	reg = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags);
>  	free(regname);
> -	return reg;
> +	return reg < 0 ? -1 : reg;

[Severity: High]
This is a pre-existing issue, but does consistently returning -1 here make it
easier to trigger an out-of-bounds stack read downstream in
check_matching_type()?

When processing an instruction with a memory reference, if register parsing
fails, the register variable can remain set to -1. Because the memory
reference flag (op_loc->mem_ref) remains true, the code can still proceed to
find_data_type() and check_matching_type().

Inside check_matching_type(), this register value (-1) is used directly as
an array index without bounds checking:

tools/perf/util/annotate-data.c:check_matching_type() {
        int reg = dloc->op->reg1;
        ...
        pr_debug_dtp("chk [%x] reg%d offset=%s%#x ok=%d kind=%d ",
                     insn_offset, reg, offset_sign, abs(offset),
                     state->regs[reg].ok, state->regs[reg].kind);

        if (!state->regs[reg].ok)
        ...
}

Could this index read uninitialized memory preceding the state->regs array?
If the stack garbage is interpreted as TSR_KIND_TYPE, might it try to
dereference it as a Dwarf_Die object and cause a segmentation fault when
analyzing maliciously crafted perf.data or ELF files?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788872630.git.wutengda@huaweicloud.com?part=5