* [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"
@ 2022-09-05 4:54 ` Athira Rajeev
0 siblings, 0 replies; 12+ messages in thread
From: Athira Rajeev @ 2022-09-05 4:54 UTC (permalink / raw)
To: acme, jolsa; +Cc: mpe, linux-perf-users, linuxppc-dev, maddy, rnsastry, kjain
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"
@ 2022-09-05 4:54 ` Athira Rajeev
0 siblings, 0 replies; 12+ messages in thread
From: Athira Rajeev @ 2022-09-05 4:54 UTC (permalink / raw)
To: acme, jolsa; +Cc: maddy, rnsastry, linux-perf-users, kjain, linuxppc-dev
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] tools/perf: Fix out of bound access to cpu mask array
2022-09-05 4:54 ` Athira Rajeev
@ 2022-09-05 4:54 ` Athira Rajeev
-1 siblings, 0 replies; 12+ messages in thread
From: Athira Rajeev @ 2022-09-05 4:54 UTC (permalink / raw)
To: acme, jolsa; +Cc: mpe, linux-perf-users, linuxppc-dev, maddy, rnsastry, kjain
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] tools/perf: Fix out of bound access to cpu mask array
@ 2022-09-05 4:54 ` Athira Rajeev
0 siblings, 0 replies; 12+ messages in thread
From: Athira Rajeev @ 2022-09-05 4:54 UTC (permalink / raw)
To: acme, jolsa; +Cc: maddy, rnsastry, linux-perf-users, kjain, linuxppc-dev
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"
2022-09-05 4:54 ` Athira Rajeev
@ 2022-09-05 6:56 ` R Nageswara Sastry
-1 siblings, 0 replies; 12+ messages in thread
From: R Nageswara Sastry @ 2022-09-05 6:56 UTC (permalink / raw)
To: Athira Rajeev, acme, jolsa
Cc: mpe, linux-perf-users, linuxppc-dev, maddy, kjain
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"
@ 2022-09-05 6:56 ` R Nageswara Sastry
0 siblings, 0 replies; 12+ messages in thread
From: R Nageswara Sastry @ 2022-09-05 6:56 UTC (permalink / raw)
To: Athira Rajeev, acme, jolsa; +Cc: linux-perf-users, maddy, linuxppc-dev, kjain
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tools/perf: Fix out of bound access to cpu mask array
2022-09-05 4:54 ` Athira Rajeev
@ 2022-09-05 6:56 ` R Nageswara Sastry
-1 siblings, 0 replies; 12+ messages in thread
From: R Nageswara Sastry @ 2022-09-05 6:56 UTC (permalink / raw)
To: Athira Rajeev, acme, jolsa
Cc: mpe, linux-perf-users, linuxppc-dev, maddy, kjain
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tools/perf: Fix out of bound access to cpu mask array
@ 2022-09-05 6:56 ` R Nageswara Sastry
0 siblings, 0 replies; 12+ messages in thread
From: R Nageswara Sastry @ 2022-09-05 6:56 UTC (permalink / raw)
To: Athira Rajeev, acme, jolsa; +Cc: linux-perf-users, maddy, linuxppc-dev, kjain
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"
2022-09-05 4:54 ` Athira Rajeev
@ 2022-09-05 10:00 ` Jiri Olsa
-1 siblings, 0 replies; 12+ messages in thread
From: Jiri Olsa @ 2022-09-05 10:00 UTC (permalink / raw)
To: Athira Rajeev
Cc: acme, mpe, linux-perf-users, linuxppc-dev, maddy, rnsastry, kjain
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"
@ 2022-09-05 10:00 ` Jiri Olsa
0 siblings, 0 replies; 12+ messages in thread
From: Jiri Olsa @ 2022-09-05 10:00 UTC (permalink / raw)
To: Athira Rajeev
Cc: maddy, rnsastry, acme, linux-perf-users, kjain, linuxppc-dev
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"
2022-09-05 10:00 ` Jiri Olsa
@ 2022-09-05 11:01 ` Athira Rajeev
-1 siblings, 0 replies; 12+ messages in thread
From: Athira Rajeev @ 2022-09-05 11:01 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Michael Ellerman, linux-perf-users,
linuxppc-dev, maddy, Nageswara Sastry, kjain
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus"
@ 2022-09-05 11:01 ` Athira Rajeev
0 siblings, 0 replies; 12+ messages in thread
From: Athira Rajeev @ 2022-09-05 11:01 UTC (permalink / raw)
To: Jiri Olsa
Cc: maddy, Nageswara Sastry, Arnaldo Carvalho de Melo,
linux-perf-users, kjain, linuxppc-dev
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-09-05 11:04 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-05 4:54 [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus" Athira Rajeev
2022-09-05 4:54 ` Athira Rajeev
2022-09-05 4:54 ` [PATCH 2/2] tools/perf: Fix out of bound access to cpu mask array Athira Rajeev
2022-09-05 4:54 ` Athira Rajeev
2022-09-05 6:56 ` R Nageswara Sastry
2022-09-05 6:56 ` R Nageswara Sastry
2022-09-05 6:56 ` [PATCH 1/2] tools/perf: Fix out of bound access to affinity "sched_cpus" R Nageswara Sastry
2022-09-05 6:56 ` R Nageswara Sastry
2022-09-05 10:00 ` Jiri Olsa
2022-09-05 10:00 ` Jiri Olsa
2022-09-05 11:01 ` Athira Rajeev
2022-09-05 11:01 ` Athira Rajeev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.