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

On 2026/8/8 21:05, [email protected] wrote:
> 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?
> 

Agreed. I need to move the initialization of memory_ref_char earlier in this patch.

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