Re: [PATCH v3 02/12] perf annotate: Handle absolute addressing in data type profiling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] perf annotate: unhandled zero address in
  __hist_entry__get_data_type()
--

commit 4e70868608c9afc77ab4fffa4bb7f6aa84106803
Author: Arnaldo Carvalho de Melo <acme@kernel.org>

perf annotate: Handle absolute addressing in data type profiling

This commit adds support for bare numeric operands as absolute memory
addresses in x86 data type profiling and guards frame-base register
comparisons in instruction tracking against -1.

> diff --git a/tools/perf/util/annotate-arch/annotate-x86.c b/tools/perf/util/annotate-arch/annotate-x86.c
> index e38a4e7a54d55..297f2fe9bec79 100644
> --- a/tools/perf/util/annotate-arch/annotate-x86.c
> +++ b/tools/perf/util/annotate-arch/annotate-x86.c
[ ... ]
> @@ -960,6 +988,11 @@ static void extract_op_location_x86(const struct arch *arch,
>  			op_loc->offset = strtol(s + 1, &p, 0);
>  			if (p && p != s + 1)
>  				op_loc->imm = true;
> +		} else if (isdigit(*s) && !ins__is_jump(&dl->ins) &&
> +			   !ins__is_call(&dl->ins)) {
> +			/* Absolute address, as used by 32-bit code */
> +			op_loc->mem_ref = true;
> +			op_loc->offset = strtoull(s, NULL, 0);
>  		}
>  	}
>  }
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 86a4beec6528e..56fffe598634c 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -2605,6 +2605,19 @@ int arch__dwarf_regnum(const struct arch *arch, const char *str)
>  	return reg < 0 ? -1 : reg;
>  }
>  
> +bool annotated_op_loc__is_abs(const struct annotated_op_loc *op_loc)
> +{
> +	return op_loc->mem_ref && op_loc->reg1 == -1 && op_loc->reg2 == -1 &&
> +	       op_loc->segment == INSN_SEG_NONE && op_loc->offset != 0;
> +}
[ ... ]
> @@ -2894,6 +2907,11 @@ __hist_entry__get_data_type(struct hist_entry *he, const struct arch *arch,
>  			dloc.var_addr = annotate_calc_pcrel(ms, dloc.ip,
>  							    op_loc->offset, dl);
>  		}
> +		/* Absolute addressing, as used by 32-bit code */
> +		else if (annotated_op_loc__is_abs(op_loc)) {
> +			dloc.var_addr = annotated_op_loc__abs_addr(op_loc, map__dso(ms->map));
> +			op_loc->reg1 = DWARF_REG_PC;
> +		}
>  
>  		/* This CPU access in kernel - pretend PC-relative addressing */
>  		if (dso__kernel(map__dso(ms->map)) && arch__is_x86(arch) &&

[Severity: Medium]
Can an unrelocated bare 0x0 operand cause an out-of-bounds read of
state->regs[-1] in check_matching_type()?

extract_op_location_x86() sets op_loc->mem_ref = true even when the parsed
address is 0, so the early !op_loc->mem_ref check in
__hist_entry__get_data_type() no longer skips the operand.  Because
annotated_op_loc__is_abs() requires op_loc->offset != 0, op_loc->reg1 is
left as -1 when __hist_entry__get_data_type() calls find_data_type(&dloc).

In find_data_type_die(), is_fbreg = (reg == fbreg) is unguarded and
matches (-1 == -1) when fbreg is -1, and if no scope variable matches,
find_data_type_block() -> find_data_type_insn() -> check_matching_type()
is reached with reg == -1:

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)
		goto check_non_register;
    ...
check_non_register:
	if (reg == dloc->fbreg || reg == state->stack_reg) {
    ...
}

This reads state->regs[-1] before struct type_state on the stack (passing
&state->regs[-1].type to DWARF helpers if the adjacent byte is non-zero)
and otherwise matches reg == dloc->fbreg (-1 == -1) at check_non_register.

Only setting op_loc->mem_ref = true in extract_op_location_x86() when the
parsed offset is non-zero (or skipping mem_ref operands with reg1 == -1 in
__hist_entry__get_data_type()) would avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115014102.2879372-1-acme@kernel.org?part=2