Re: [PATCH v2 3/4] perf annotate-date: Allow out-of-size access for flex-array types

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] check_variable and check_matching_type fail to pass the typedef-resolved 'sized_type' to 'check_type_offset', causing an unused variable warning and breaking flex-array detection for typedefs.
- [Medium] Uninitialized stack variable 'last_mb' is used in 'die_has_flex_array_recurse' if a structure contains children but no data members.
--

commit cd675d28ffa1b31e0fe343fa27c89e7794eb74ec
Author: Namhyung Kim <namhyung@kernel.org>

perf annotate-date: Allow out-of-size access for flex-array types
    
Structs that have a flex array will have accesses beyond its original
size as the array was declared as 0 sized.  For now, it just allow any
offset bigger than the size.  It could be refined later.

> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index aff60a630fd05..e8aff6a916c0a 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c

[ ... ]

> @@ -399,6 +407,7 @@ static struct annotated_data_type *dso__findnew_data_type(struct dso *dso,
>  	result->self.type_name = type_name;
>  	result->self.size = size;
>  	INIT_LIST_HEAD(&result->self.children);
> +	result->flex_array = die_has_flex_array(type_die);

[Severity: Medium]
Will this call into die_has_flex_array_recurse() trigger an uninitialized
stack variable access for empty structures?

If the structure has child DIEs but none of them are DW_TAG_member, the
last_mb variable won't be assigned in the loop, leaving it uninitialized
when evaluated:

tools/perf/util/dwarf-aux.c:die_has_flex_array_recurse() {
	Dwarf_Die die_mem, last_mb;
...
	do {
		if (dwarf_tag(&die_mem) != DW_TAG_member)
			continue;
...
		if (tag == DW_TAG_structure_type)
			memcpy(&last_mb, &die_mem, sizeof(last_mb));

	} while (dwarf_siblingof(&die_mem, &die_mem) == 0);

	if (tag == DW_TAG_structure_type) {
		if (is_flex_array_member(&last_mb))
			return true;
...

Should last_mb be initialized, or should we track whether it was set before
calling is_flex_array_member()?

>  
>  	if (symbol_conf.annotate_data_member)
>  		add_member_types(result, type_die);

[ ... ]

> @@ -554,15 +580,7 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
>  	else
>  		sized_type = *type_die;
>  
> -	/* Get the size of the actual type */
> -	if (dwarf_aggregate_size(&sized_type, &size) < 0)
> -		return PERF_TMR_NO_SIZE;
> -
> -	/* Minimal sanity check */
> -	if ((unsigned)offset >= size)
> -		return PERF_TMR_BAD_OFFSET;
> -
> -	return PERF_TMR_OK;
> +	return check_type_offset(type_die, offset);
>  }

[Severity: Medium]
Should this pass &sized_type instead of type_die?

Passing type_die ignores the typedef resolution performed earlier in this
function and leaves sized_type completely unused. Since check_type_offset()
now evaluates die_has_flex_array(type_die), this might break flex-array
detection for types hidden behind typedefs.

[ ... ]

> @@ -1190,12 +1202,7 @@ static enum type_match_result check_matching_type(struct type_state *state,
>  
>  		dloc->type_offset = dloc->op->offset + state->regs[reg].offset;
>  
> -		/* Get the size of the actual type */
> -		if (dwarf_aggregate_size(type_die, &size) < 0 ||
> -		    (unsigned)dloc->type_offset >= size)
> -			return PERF_TMR_BAD_OFFSET;
> -
> -		return PERF_TMR_OK;
> +		return check_type_offset(type_die, dloc->type_offset);
>  	}

[Severity: Medium]
For pointer types here, should type_die also be stripped of typedefs before
checking the type offset?

Similar to the logic used for TSR_KIND_TYPE, flex-arrays might go undetected
if they are hidden behind unresolved typedefs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914064535.1671939-1-namhyung@kernel.org?part=3