Re: [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 05-Sep-2022, at 3:30 PM, Jiri Olsa <olsajiri@gmail.com> wrote:
> 
> On Mon, Sep 05, 2022 at 10:24:40AM +0530, Athira Rajeev wrote:
>> The affinity code in "affinity_set" function access array
>> named "sched_cpus". The size for this array is allocated in
>> affinity_setup function which is nothing but value from
>> get_cpu_set_size. This is used to contain the cpumask value
>> for each cpu. While setting bit for each cpu, it calls
>> "set_bit" function which access index in sched_cpus array.
>> If we provide a command-line option to -C which is more than
>> the number of CPU's present in the system, the set_bit could
>> access an array member which is out-of the array size. This
>> is because currently, there is no boundary check for the CPU.
>> This will result in seg fault:
>> 
>> <<>>
>> ./perf stat -C 12323431 ls
>> Perf can support 2048 CPUs. Consider raising MAX_NR_CPUS
>> Segmentation fault (core dumped)
>> <<>>
>> 
>> Fix this by adding boundary check for the array.
>> 
>> After the fix from powerpc system:
>> 
>> <<>>
>> ./perf stat -C 12323431 ls 1>out
>> Perf can support 2048 CPUs. Consider raising MAX_NR_CPUS
>> 
>> Performance counter stats for 'CPU(s) 12323431':
>> 
>>   <not supported> msec cpu-clock
>>   <not supported>      context-switches
>>   <not supported>      cpu-migrations
>>   <not supported>      page-faults
>>   <not supported>      cycles
>>   <not supported>      instructions
>>   <not supported>      branches
>>   <not supported>      branch-misses
>> 
>>       0.001192373 seconds time elapsed
>> <<>>
>> 
>> Reported-by: Nageswara Sastry <rnsastry@linux.ibm.com>
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> tools/perf/util/affinity.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tools/perf/util/affinity.c b/tools/perf/util/affinity.c
>> index 4d216c0dc425..a1dd37347abc 100644
>> --- a/tools/perf/util/affinity.c
>> +++ b/tools/perf/util/affinity.c
>> @@ -49,8 +49,14 @@ void affinity__set(struct affinity *a, int cpu)
>> {
>> 	int cpu_set_size = get_cpu_set_size();
>> 
>> -	if (cpu == -1)
>> +	/*
>> +	 * Return:
>> +	 * - if cpu is -1
>> +	 * - restrict out of bound access to sched_cpus
>> +	 */
>> +	if (cpu == -1 || ((cpu / __BITS_PER_LONG) >= (cpu_set_size / 8)))
> 
> hm, there's __BITS_PER_LONG in one case, but then there's hardcoded 8
> 
> would this be simpler:
> 
> 	if (cpu == -1 || ((cpu >= (cpu_set_size * 8))))
> 		return;
> 
> jirka

Hi Jiri,

Thanks for the review. I will post a V2 with this change

Athira

> 
>> 		return;
>> +
>> 	a->changed = true;
>> 	set_bit(cpu, a->sched_cpus);
>> 	/*
>> -- 
>> 2.35.1