Re: [PATCH 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 3 potential issue(s) to consider:

New issues:
- [Low] The patch breaks the negative bounds check for variables containing flexible arrays, allowing invalid negative offsets to be falsely accepted.
- [Low] Failure to expand flexible array elements if the element type is a typedef.

Pre-existing issues:
- [Low] Missing explicit inclusion of <string.h> for libc string functions, violating musl compatibility subsystem guidelines.
--

commit 71730adfd191d6d7dd60b7b99d73c9bc2ede7031
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..ad043403ced58 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -8,5 +8,5 @@
>  #include <errno.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <inttypes.h>
>  #include <linux/zalloc.h>

[Severity: Low]
This is a pre-existing issue, but is there a missing include for <string.h>
in tools/perf/util/annotate-data.c? The file uses libc string functions like
strcmp and strdup, and failing to explicitly include <string.h> violates the
subsystem guidelines for musl libc compatibility, which could lead to build
failures.

[ ... ]

> @@ -248,8 +248,15 @@ static int __add_member_cb(Dwarf_Die *die, void *arg)
>  	else
>  		die_mem = member_type;
>  
> -	if (dwarf_aggregate_size(&die_mem, &size) < 0)
> -		size = 0;
> +	if (dwarf_aggregate_size(&die_mem, &size) < 0) {
> +		if (dwarf_tag(&die_mem) == DW_TAG_array_type) { /* flex-array? */
> +			__die_get_real_type(&die_mem, &die_mem);

[Severity: Low]
Will __add_member_cb() fail to resolve typedefs here? Using
__die_get_real_type instead of die_get_real_type strips const and volatile
qualifiers but stops at DW_TAG_typedef. This prevents the subsequent struct
expansion if the flexible array element type is a typedef of a struct.

> +			if (dwarf_aggregate_size(&die_mem, &size) < 0)
> +				size = 0;
> +		} else {
> +			size = 0;
> +		}
> +	}
>  
>  	if (dwarf_attr_integrate(die, DW_AT_data_member_location, &attr)) {

[ ... ]

> @@ -559,7 +567,7 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
>  		return PERF_TMR_NO_SIZE;
>  
>  	/* Minimal sanity check */
> -	if ((unsigned)offset >= size)
> +	if ((unsigned)offset >= size && !die_has_flex_array(&sized_type))

[Severity: Low]
Does check_variable() allow invalid negative offsets to bypass the check?
Since offset is cast to unsigned, a negative offset evaluates to a large
positive number. If the type has a flexible array, the negative bounds check
is now broken and the offset will be falsely accepted.

>  		return PERF_TMR_BAD_OFFSET;
>  
>  	return PERF_TMR_OK;

[ ... ]

> @@ -1168,7 +1176,7 @@ static enum type_match_result check_matching_type(struct type_state *state,
>  
>  		/* Get the size of the actual type */
>  		if (dwarf_aggregate_size(&sized_type, &size) < 0 ||
> -		    (unsigned)dloc->type_offset >= size)
> +		    ((unsigned)dloc->type_offset >= size && !die_has_flex_array(&sized_type)))

[Severity: Low]
Could this logic in check_matching_type() lead to negative offsets bypassing
the check as well? Similar to check_variable(), casting dloc->type_offset to
unsigned causes negative values to appear as large positive numbers, bypassing
the bounds check if there is a flexible array. This flaw is repeated multiple
times in the pointer tracking logic.

>  			return PERF_TMR_BAD_OFFSET;
>  
>  		return PERF_TMR_OK;

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