Re: [PATCH v4 12/23] perf annotate-arm64: Enable instruction tracking support

On 2026/8/8 21:22, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Missing bounds check on the secondary destination register (`dst->reg2`) leads to an out-of-bounds stack write in `update_insn_state_arm64()`.
> - [Medium] Incorrect invalidation of base pointer type information during memory store instructions in `update_insn_state_arm64()`.
> --
> 
> --- Patch [12]: [PATCH v4 12/23] perf annotate-arm64: Enable instruction tracking support ---
> commit f55f5ca2b14376eae146cdb6373ed5a53a114257
> Author: Tengda Wu <[email protected]>
> 
> perf annotate-arm64: Enable instruction tracking support
> 
> Enable basic instruction tracking for arm64 by implementing three
> essential functions in the find_data_type_block() call path.
> 
>> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
>> index 6b4f1228afa9b..44daa0176e512 100644
>> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
>> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
>> @@ -353,6 +357,65 @@ static int extract_op_location_arm64(const struct arch *arch,
> 
> [ ... ]
> 
>> +	/*
>> +	 * Invalidate destination register(s) for unsupported instructions to
>> +	 * prevent stale type info from propagating to subsequent instructions.
>> +	 */
>> +	if (has_reg_type(state, dst->reg1)) {
>> +		pr_debug_dtp("%s [%x] invalidate reg%d",
>> +			     dl->ins.name, insn_offset, dst->reg1);
>> +		invalidate_reg_state(&state->regs[dst->reg1]);
> 
> [Severity: Medium]
> Does this incorrectly invalidate the base pointer type information during memory
> store instructions?
> 
> If dst->mem_ref is true (e.g., for a store instruction like str x1, [x0]),
> this fallback lacks a !dst->mem_ref check. Will it incorrectly invalidate the
> extracted base register (x0), assuming it is being overwritten?
> 

Agreed.

>> +		if (dst->multi_regs) {
>> +			pr_debug_dtp(" and reg%d", dst->reg2);
>> +			invalidate_reg_state(&state->regs[dst->reg2]);
> 
> [Severity: High]
> Can this cause an out-of-bounds write on the stack-allocated type_state
> structure?
> 
> If an instruction parses with multiple registers where the second is invalid
> (e.g., ldp x0, x99, [sp]), extract_op_location_arm64() sets op_loc->reg2
> to a negative error code since it cannot find the register name.
> 
> Because there is no check for dst->reg2 >= 0 here, does this result in a
> negative index being used for the regs array?
> 

Agreed, I need to add a has_reg_type(state, dst->reg2) check.

>> +		}
>> +		pr_debug_dtp("\n");
>> +		return;
>> +	}
>> +}
>