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

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?

> +		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?

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

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