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

[Anshuman Khandual <anshuman.khandual@arm.com>]

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>