Re: [PATCH v4 03/23] perf annotate-arm64: Generalize arm64_mov__parse to support more instructions

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arch->objdump.memory_ref_char is uninitialized (0) for ARM64, causing arm64__check_multi_regs() to fail parsing memory operands and creating an out-of-bounds read risk.
--

--- Patch [3]: [PATCH v4 03/23] perf annotate-arm64: Generalize arm64_mov__parse to support more instructions ---
commit 0546b270cb844bdab4fcf529e313bb0ead4fc3f8
Author: Tengda Wu <[email protected]>

perf annotate-arm64: Generalize arm64_mov__parse to support more instructions
    
As the default parser for arm64 instructions, arm64_mov__parse()
currently only supports parsing instructions with the 'addr <symbol>'
suffix. Other instructions are not supported, causing a lack of
source and target information for most regular instructions.
    
Generalize arm64_mov__parse to extend its parsing capabilities:
1. Parse 'ops->target.raw' and 'ops->source.raw' by default.
2. Utilize arm64__check_multi_regs() to identify if source and
   target contain multiple registers.
3. Maintain backward compatibility for instructions containing the
   'addr <symbol>' suffix.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 33080fdca125f..5e824fd617ee5 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c

[ ... ]

> +static bool arm64__check_multi_regs(const struct arch *arch, const char *op)
> +{
> +	const char *p = op;
> +	int reg_count = 0;
> +
> +	while (p && *p) {
> +		p = skip_spaces(p);
> +		if (*p == arch->objdump.memory_ref_char)
> +			p++;

[Severity: Medium]
Could this result in an out-of-bounds read?

Since arch->objdump.memory_ref_char is not explicitly initialized for ARM64,
it defaults to 0. If the operand string contains trailing spaces,
skip_spaces() will advance p to the null terminator. 

This would cause the following check to match:

    if (*p == arch->objdump.memory_ref_char) /* 0 == 0 */
        p++;

This increments p past the null terminator, and the subsequent call to
arm64__is_reg(p) would access unallocated memory:

    static bool arm64__is_reg(const char *op)
    {
        ...
        if ((op[0] == 'x' || op[0] == 'w') && isdigit(op[1]))
        ...

Should memory_ref_char be initialized to '[' for ARM64 so that memory
operands are correctly parsed?

> +
> +		if (arm64__is_reg(p))
> +			reg_count++;
> +

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