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

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

[Athira Rajeev]

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)))
 		return;
+
 	a->changed = true;
 	set_bit(cpu, a->sched_cpus);
 	/*
-- 
2.35.1

[PATCH 2/2] tools/perf: Fix out of bound access to cpu mask array

[Athira Rajeev]

The cpu mask init code in "record__mmap_cpu_mask_init"
function access "bits" array part of "struct mmap_cpu_mask".
The size of this array is the value from cpu__max_cpu().cpu.
This array 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 "bits" array. If we provide a command
line option to -C which is greater 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 record -C 12341234 ls
Perf can support 2048 CPUs. Consider raising MAX_NR_CPUS
Segmentation fault (core dumped)
<<>>

Debugging with gdb, points to function flow as below:

<<>>
set_bit
record__mmap_cpu_mask_init
record__init_thread_default_masks
record__init_thread_masks
cmd_record
<<>>

Fix this by adding boundary check for the array.

After the patch:
<<>>
./perf record -C 12341234 ls
Perf can support 2048 CPUs. Consider raising MAX_NR_CPUS
Failed to initialize parallel data streaming masks
<<>>

With this fix, if -C is given a non-exsiting CPU, perf
record will fail with:

<<>>
 ./perf record -C 50 ls
Failed to initialize parallel data streaming masks
<<>>

Reported-by: Nageswara Sastry <rnsastry@linux.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/builtin-record.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 4713f0f3a6cf..09b68d76bbdc 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -3358,16 +3358,22 @@ static struct option __record_options[] = {
 
 struct option *record_options = __record_options;
 
-static void record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_cpu_map *cpus)
+static int record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_cpu_map *cpus)
 {
 	struct perf_cpu cpu;
 	int idx;
 
 	if (cpu_map__is_dummy(cpus))
-		return;
+		return 0;
 
-	perf_cpu_map__for_each_cpu(cpu, idx, cpus)
+	perf_cpu_map__for_each_cpu(cpu, idx, cpus) {
+		/* Return ENODEV is input cpu is greater than max cpu */
+		if ((unsigned long)cpu.cpu > mask->nbits)
+			return -ENODEV;
 		set_bit(cpu.cpu, mask->bits);
+	}
+
+	return 0;
 }
 
 static int record__mmap_cpu_mask_init_spec(struct mmap_cpu_mask *mask, const char *mask_spec)
@@ -3379,7 +3385,9 @@ static int record__mmap_cpu_mask_init_spec(struct mmap_cpu_mask *mask, const cha
 		return -ENOMEM;
 
 	bitmap_zero(mask->bits, mask->nbits);
-	record__mmap_cpu_mask_init(mask, cpus);
+	if (record__mmap_cpu_mask_init(mask, cpus))
+		return -ENODEV;
+
 	perf_cpu_map__put(cpus);
 
 	return 0;
@@ -3461,7 +3469,12 @@ static int record__init_thread_masks_spec(struct record *rec, struct perf_cpu_ma
 		pr_err("Failed to allocate CPUs mask\n");
 		return ret;
 	}
-	record__mmap_cpu_mask_init(&cpus_mask, cpus);
+
+	ret = record__mmap_cpu_mask_init(&cpus_mask, cpus);
+	if (ret) {
+		pr_err("Failed to init cpu mask\n");
+		goto out_free_cpu_mask;
+	}
 
 	ret = record__thread_mask_alloc(&full_mask, cpu__max_cpu().cpu);
 	if (ret) {
@@ -3702,7 +3715,8 @@ static int record__init_thread_default_masks(struct record *rec, struct perf_cpu
 	if (ret)
 		return ret;
 
-	record__mmap_cpu_mask_init(&rec->thread_masks->maps, cpus);
+	if (record__mmap_cpu_mask_init(&rec->thread_masks->maps, cpus))
+		return -ENODEV;
 
 	rec->nr_threads = 1;
 
-- 
2.35.1

Re: [PATCH 2/2] tools/perf: Fix out of bound access to cpu mask array

[R Nageswara Sastry]

On 05/09/22 10:24 am, Athira Rajeev wrote:
> The cpu mask init code in "record__mmap_cpu_mask_init"
> function access "bits" array part of "struct mmap_cpu_mask".
> The size of this array is the value from cpu__max_cpu().cpu.
> This array 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 "bits" array. If we provide a command
> line option to -C which is greater 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 record -C 12341234 ls
> Perf can support 2048 CPUs. Consider raising MAX_NR_CPUS
> Segmentation fault (core dumped)
> <<>>
> 
> Debugging with gdb, points to function flow as below:
> 
> <<>>
> set_bit
> record__mmap_cpu_mask_init
> record__init_thread_default_masks
> record__init_thread_masks
> cmd_record
> <<>>
> 
> Fix this by adding boundary check for the array.
> 
> After the patch:
> <<>>
> ./perf record -C 12341234 ls
> Perf can support 2048 CPUs. Consider raising MAX_NR_CPUS
> Failed to initialize parallel data streaming masks
> <<>>
> 
> With this fix, if -C is given a non-exsiting CPU, perf
> record will fail with:
> 
> <<>>
>   ./perf record -C 50 ls
> Failed to initialize parallel data streaming masks
> <<>>
> 
> Reported-by: Nageswara Sastry <rnsastry@linux.ibm.com>

Tested-by: Nageswara Sastry <rnsastry@linux.ibm.com>

> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>   tools/perf/builtin-record.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 4713f0f3a6cf..09b68d76bbdc 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -3358,16 +3358,22 @@ static struct option __record_options[] = {
>   
>   struct option *record_options = __record_options;
>   
> -static void record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_cpu_map *cpus)
> +static int record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_cpu_map *cpus)
>   {
>   	struct perf_cpu cpu;
>   	int idx;
>   
>   	if (cpu_map__is_dummy(cpus))
> -		return;
> +		return 0;
>   
> -	perf_cpu_map__for_each_cpu(cpu, idx, cpus)
> +	perf_cpu_map__for_each_cpu(cpu, idx, cpus) {
> +		/* Return ENODEV is input cpu is greater than max cpu */
> +		if ((unsigned long)cpu.cpu > mask->nbits)
> +			return -ENODEV;
>   		set_bit(cpu.cpu, mask->bits);
> +	}
> +
> +	return 0;
>   }
>   
>   static int record__mmap_cpu_mask_init_spec(struct mmap_cpu_mask *mask, const char *mask_spec)
> @@ -3379,7 +3385,9 @@ static int record__mmap_cpu_mask_init_spec(struct mmap_cpu_mask *mask, const cha
>   		return -ENOMEM;
>   
>   	bitmap_zero(mask->bits, mask->nbits);
> -	record__mmap_cpu_mask_init(mask, cpus);
> +	if (record__mmap_cpu_mask_init(mask, cpus))
> +		return -ENODEV;
> +
>   	perf_cpu_map__put(cpus);
>   
>   	return 0;
> @@ -3461,7 +3469,12 @@ static int record__init_thread_masks_spec(struct record *rec, struct perf_cpu_ma
>   		pr_err("Failed to allocate CPUs mask\n");
>   		return ret;
>   	}
> -	record__mmap_cpu_mask_init(&cpus_mask, cpus);
> +
> +	ret = record__mmap_cpu_mask_init(&cpus_mask, cpus);
> +	if (ret) {
> +		pr_err("Failed to init cpu mask\n");
> +		goto out_free_cpu_mask;
> +	}
>   
>   	ret = record__thread_mask_alloc(&full_mask, cpu__max_cpu().cpu);
>   	if (ret) {
> @@ -3702,7 +3715,8 @@ static int record__init_thread_default_masks(struct record *rec, struct perf_cpu
>   	if (ret)
>   		return ret;
>   
> -	record__mmap_cpu_mask_init(&rec->thread_masks->maps, cpus);
> +	if (record__mmap_cpu_mask_init(&rec->thread_masks->maps, cpus))
> +		return -ENODEV;
>   
>   	rec->nr_threads = 1;
>   

-- 
Thanks and Regards
R.Nageswara Sastry

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

[R Nageswara Sastry]

On 05/09/22 10:24 am, 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>

Tested-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)))
>   		return;
> +
>   	a->changed = true;
>   	set_bit(cpu, a->sched_cpus);
>   	/*

-- 
Thanks and Regards
R.Nageswara Sastry

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

[Jiri Olsa]

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

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

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

[Athira Rajeev]

> 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