Re: [PATCH] perf branch: Fix heap out-of-bounds write in branch_type_count()

Re: [PATCH] perf branch: Fix heap out-of-bounds write in branch_type_count()

[Anshuman Khandual]

On 09/08/25 3:08 PM, Yujun Dong wrote:
> The branch_type_count() function writes to st->new_counts[flags->new_type]
> when flags->type is PERF_BR_EXTEND_ABI. However, while the array
> st->new_counts is sized for PERF_BR_NEW_MAX (8) entries, the field
> flags->new_type is a 4-bit unsigned value and may hold values up to 15.
> 
> This mismatch allows crafted perf data to trigger a heap out-of-bounds
> write when flags->new_type >= 8, leading to memory corruption.

Crafted ? How could flags->new_type >= 8 when PERF_BR_NEW_MAX is capped at 8.
Is this a real scenario that happened on a system ?

> 
> Add a bounds check to ensure flags->new_type is less than
> PERF_BR_NEW_MAX before accessing the new_counts array.

But it might make sense to add this check just to be on the safer side.

> 
> Fixes: 0ddea8e2a0c2 ("perf branch: Extend branch type classification")
> Signed-off-by: Yujun Dong <yujundong@pascal-lab.net>
> ---
>  tools/perf/util/branch.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/branch.c b/tools/perf/util/branch.c
> index 3712be067464..8ea6628c7735 100644
> --- a/tools/perf/util/branch.c
> +++ b/tools/perf/util/branch.c
> @@ -21,10 +21,12 @@ void branch_type_count(struct branch_type_stat *st, struct branch_flags *flags,
>  	if (flags->type == PERF_BR_UNKNOWN || from == 0)
>  		return;
>  
> -	if (flags->type == PERF_BR_EXTEND_ABI)
> -		st->new_counts[flags->new_type]++;
> -	else
> +	if (flags->type == PERF_BR_EXTEND_ABI) {
> +		if (flags->new_type < PERF_BR_NEW_MAX)
> +			st->new_counts[flags->new_type]++;
> +	} else {
>  		st->counts[flags->type]++;
> +	}
>  
>  	if (flags->type == PERF_BR_COND) {
>  		if (to > from)

Re: [PATCH] perf branch: Fix heap out-of-bounds write in branch_type_count()

[Yujun Dong]

On 2025/8/11 15:03, Anshuman Khandual wrote:
 > On 09/08/25 3:08 PM, Yujun Dong wrote:
 > > The branch_type_count() function writes to 
st->new_counts[flags->new_type]
 > > when flags->type is PERF_BR_EXTEND_ABI. However, while the array
 > > st->new_counts is sized for PERF_BR_NEW_MAX (8) entries, the field
 > > flags->new_type is a 4-bit unsigned value and may hold values up to 15.
 > >
 > > This mismatch allows crafted perf data to trigger a heap out-of-bounds
 > > write when flags->new_type >= 8, leading to memory corruption.
 >
 > Crafted ? How could flags->new_type >= 8 when PERF_BR_NEW_MAX is 
capped at 8.
 > Is this a real scenario that happened on a system ?
 >

Thanks for your review and for raising that question. The new_type field
in struct branch_flags is declared as a 4-bit bitfield (u64 new_type:4),
meaning it can hold values from 0 to 15, even though PERF_BR_NEW_MAX is
defined as 8. So, it's entirely possible for flags->new_type to be >= 8.

In fact, I've observed such cases when running real-world perf record/top,
where perf.data produced contains invalid new_type values, likely due to
other bugs or unexpected data corruption. Additionally, a maliciously
crafted perf.data file can also force this out-of-bounds write.

 > >
 > > Add a bounds check to ensure flags->new_type is less than
 > > PERF_BR_NEW_MAX before accessing the new_counts array.
 >
 > But it might make sense to add this check just to be on the safer side.
 >

Notably, new_type is only used in two places:
1. In branch_new_type_name(), where the bounds are already validated.
2. In branch_type_count(), where the current patch now adds the
necessary check.

Admittedly, the mismatch between the bit-field width (0-15) and
PERF_BR_NEW_MAX (8) is the root cause. While adjusting the bit-field
to match PERF_BR_NEW_MAX would also resolve the mismatch, that risks
breaking existing compatibility. Therefore, adding a bounds check at
the use site is the least disruptive correction.

 > >
 > > Fixes: 0ddea8e2a0c2 ("perf branch: Extend branch type classification")
 > > Signed-off-by: Yujun Dong <yujundong@pascal-lab.net>
 > > ---
 > >  tools/perf/util/branch.c | 8 +++++---
 > >  1 file changed, 5 insertions(+), 3 deletions(-)
 > >
 > > diff --git a/tools/perf/util/branch.c b/tools/perf/util/branch.c
 > > index 3712be067464..8ea6628c7735 100644
 > > --- a/tools/perf/util/branch.c
 > > +++ b/tools/perf/util/branch.c
 > > @@ -21,10 +21,12 @@ void branch_type_count(struct branch_type_stat 
*st, struct branch_flags *flags,
 > >      if (flags->type == PERF_BR_UNKNOWN || from == 0)
 > >          return;
 > >
 > > -    if (flags->type == PERF_BR_EXTEND_ABI)
 > > -        st->new_counts[flags->new_type]++;
 > > -    else
 > > +    if (flags->type == PERF_BR_EXTEND_ABI) {
 > > +        if (flags->new_type < PERF_BR_NEW_MAX)
 > > +            st->new_counts[flags->new_type]++;
 > > +    } else {
 > >          st->counts[flags->type]++;
 > > +    }
 > >
 > >      if (flags->type == PERF_BR_COND) {
 > >          if (to > from)
 >

Re: [PATCH] perf branch: Fix heap out-of-bounds write in branch_type_count()

[Anshuman Khandual]

On 12/08/25 1:01 PM, Yujun Dong wrote:
> On 2025/8/11 15:03, Anshuman Khandual wrote:
>> On 09/08/25 3:08 PM, Yujun Dong wrote:
>> > The branch_type_count() function writes to st->new_counts[flags->new_type]
>> > when flags->type is PERF_BR_EXTEND_ABI. However, while the array
>> > st->new_counts is sized for PERF_BR_NEW_MAX (8) entries, the field
>> > flags->new_type is a 4-bit unsigned value and may hold values up to 15.
>> >
>> > This mismatch allows crafted perf data to trigger a heap out-of-bounds
>> > write when flags->new_type >= 8, leading to memory corruption.
>>
>> Crafted ? How could flags->new_type >= 8 when PERF_BR_NEW_MAX is capped at 8.
>> Is this a real scenario that happened on a system ?
>>
> 
> Thanks for your review and for raising that question. The new_type field
> in struct branch_flags is declared as a 4-bit bitfield (u64 new_type:4),
> meaning it can hold values from 0 to 15, even though PERF_BR_NEW_MAX is
> defined as 8. So, it's entirely possible for flags->new_type to be >= 8.

Sure it is possible but not probable I guess as new_type itself would be
first guarded by PERF_BR_NEW_MAX.

> 
> In fact, I've observed such cases when running real-world perf record/top,
> where perf.data produced contains invalid new_type values, likely due to
> other bugs or unexpected data corruption. Additionally, a maliciously
> crafted perf.data file can also force this out-of-bounds write.

Agreed. 

> 
>> >
>> > Add a bounds check to ensure flags->new_type is less than
>> > PERF_BR_NEW_MAX before accessing the new_counts array.
>>
>> But it might make sense to add this check just to be on the safer side.
>>
> 
> Notably, new_type is only used in two places:
> 1. In branch_new_type_name(), where the bounds are already validated.
> 2. In branch_type_count(), where the current patch now adds the
> necessary check.

Agreed - this change will ensure consistency across both the functions.

> 
> Admittedly, the mismatch between the bit-field width (0-15) and
> PERF_BR_NEW_MAX (8) is the root cause. While adjusting the bit-field
> to match PERF_BR_NEW_MAX would also resolve the mismatch, that risks
> breaking existing compatibility. Therefore, adding a bounds check at
> the use site is the least disruptive correction.

Right, increasing PERF_BR_NEW_MAX to 15 will not be desirable.

> 
>> >
>> > Fixes: 0ddea8e2a0c2 ("perf branch: Extend branch type classification")
>> > Signed-off-by: Yujun Dong <yujundong@pascal-lab.net>
>> > ---
>> >  tools/perf/util/branch.c | 8 +++++---
>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/tools/perf/util/branch.c b/tools/perf/util/branch.c
>> > index 3712be067464..8ea6628c7735 100644
>> > --- a/tools/perf/util/branch.c
>> > +++ b/tools/perf/util/branch.c
>> > @@ -21,10 +21,12 @@ void branch_type_count(struct branch_type_stat *st, struct branch_flags *flags,
>> >      if (flags->type == PERF_BR_UNKNOWN || from == 0)
>> >          return;
>> >
>> > -    if (flags->type == PERF_BR_EXTEND_ABI)
>> > -        st->new_counts[flags->new_type]++;
>> > -    else
>> > +    if (flags->type == PERF_BR_EXTEND_ABI) {
>> > +        if (flags->new_type < PERF_BR_NEW_MAX)
>> > +            st->new_counts[flags->new_type]++;
>> > +    } else {
>> >          st->counts[flags->type]++;
>> > +    }
>> >
>> >      if (flags->type == PERF_BR_COND) {
>> >          if (to > from)
>> >Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>