[PATCH 1/3] perf: Return proper values for user stack errors

[PATCH 1/3] perf: Return proper values for user stack errors

[Jiri Olsa]

Return immediately when we find issue in the user
stack checks. The error value could get overwritten
by following check for PERF_SAMPLE_REGS_INTR.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index fc1c330c6bd6..4d997bc064d2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10191,9 +10191,9 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 		 * __u16 sample size limit.
 		 */
 		if (attr->sample_stack_user >= USHRT_MAX)
-			ret = -EINVAL;
+			return -EINVAL;
 		else if (!IS_ALIGNED(attr->sample_stack_user, sizeof(u64)))
-			ret = -EINVAL;
+			return -EINVAL;
 	}
 
 	if (!attr->sample_max_stack)
-- 
2.13.6

[PATCH 2/3] perf: Fix sample_max_stack maximum check

[Jiri Olsa]

The syzbot hit KASAN bug in perf_callchain_store
having the entry stored behind the allocated bounds [1].

We miss the sample_max_stack check for the initial event
that allocates callchain buffers. This missing check allows
to create an event with sample_max_stack value bigger than
the global sysctl maximum:

  # sysctl -a | grep perf_event_max_stack
  kernel.perf_event_max_stack = 127

  # perf record -vv -C 1 -e cycles/max-stack=256/ kill
  ...
  perf_event_attr:
    size                             112
    ...
    sample_max_stack                 256
  ------------------------------------------------------------
  sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 4

Note the '-C 1', which forces perf record to create just
single event. Otherwise it opens event for every cpu, then
the sample_max_stack check fails on the second event and
all's fine.

The fix is to run the sample_max_stack check also for the
first event with callchains.

[1] https://marc.info/?l=linux-kernel&m=152352732920874&w=2

Reported-by: syzbot+7c449856228b63ac951e@syzkaller.appspotmail.com
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/callchain.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
index 772a43fea825..73cc26e321de 100644
--- a/kernel/events/callchain.c
+++ b/kernel/events/callchain.c
@@ -119,19 +119,22 @@ int get_callchain_buffers(int event_max_stack)
 		goto exit;
 	}
 
+	/*
+	 * If requesting per event more than the global cap,
+	 * return a different error to help userspace figure
+	 * this out.
+	 *
+	 * And also do it here so that we have &callchain_mutex held.
+	 */
+	if (event_max_stack > sysctl_perf_event_max_stack) {
+		err = -EOVERFLOW;
+		goto exit;
+	}
+
 	if (count > 1) {
 		/* If the allocation failed, give up */
 		if (!callchain_cpus_entries)
 			err = -ENOMEM;
-		/*
-		 * If requesting per event more than the global cap,
-		 * return a different error to help userspace figure
-		 * this out.
-		 *
-		 * And also do it here so that we have &callchain_mutex held.
-		 */
-		if (event_max_stack > sysctl_perf_event_max_stack)
-			err = -EOVERFLOW;
 		goto exit;
 	}
 
-- 
2.13.6

[PATCH 3/3] perf: Remove superfluous allocation error check

[Jiri Olsa]

If the get_callchain_buffers fails to allocate the buffer
it will decrease the nr_callchain_events right away.

There's no point of checking the allocation error for
nr_callchain_events > 1. Removing that check.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/callchain.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
index 73cc26e321de..c187aa3df3c8 100644
--- a/kernel/events/callchain.c
+++ b/kernel/events/callchain.c
@@ -131,14 +131,8 @@ int get_callchain_buffers(int event_max_stack)
 		goto exit;
 	}
 
-	if (count > 1) {
-		/* If the allocation failed, give up */
-		if (!callchain_cpus_entries)
-			err = -ENOMEM;
-		goto exit;
-	}
-
-	err = alloc_callchain_buffers();
+	if (count == 1)
+		err = alloc_callchain_buffers();
 exit:
 	if (err)
 		atomic_dec(&nr_callchain_events);
-- 
2.13.6

Re: [PATCH 1/3] perf: Return proper values for user stack errors

[Arnaldo Carvalho de Melo]

Em Sun, Apr 15, 2018 at 11:23:50AM +0200, Jiri Olsa escreveu:
> Return immediately when we find issue in the user
> stack checks. The error value could get overwritten
> by following check for PERF_SAMPLE_REGS_INTR.

Applied, together with a:

Cc: Stephane Eranian <eranian@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Fixes: 60e2364e60e8 ("perf: Add ability to sample machine state on interrupt")
 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/events/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index fc1c330c6bd6..4d997bc064d2 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10191,9 +10191,9 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
>  		 * __u16 sample size limit.
>  		 */
>  		if (attr->sample_stack_user >= USHRT_MAX)
> -			ret = -EINVAL;
> +			return -EINVAL;
>  		else if (!IS_ALIGNED(attr->sample_stack_user, sizeof(u64)))
> -			ret = -EINVAL;
> +			return -EINVAL;
>  	}
>  
>  	if (!attr->sample_max_stack)
> -- 
> 2.13.6

Re: [PATCH 1/3] perf: Return proper values for user stack errors

[Arnaldo Carvalho de Melo]

Em Sun, Apr 15, 2018 at 11:23:50AM +0200, Jiri Olsa escreveu:
> Return immediately when we find issue in the user
> stack checks. The error value could get overwritten
> by following check for PERF_SAMPLE_REGS_INTR.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Also, since you're sending this to syskaller-bugs, was this something
reported by that tool? If so I think we should communicate with him,
like described in its docs, no? /me goes to find the doc snippet...



>  kernel/events/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index fc1c330c6bd6..4d997bc064d2 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10191,9 +10191,9 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
>  		 * __u16 sample size limit.
>  		 */
>  		if (attr->sample_stack_user >= USHRT_MAX)
> -			ret = -EINVAL;
> +			return -EINVAL;
>  		else if (!IS_ALIGNED(attr->sample_stack_user, sizeof(u64)))
> -			ret = -EINVAL;
> +			return -EINVAL;
>  	}
>  
>  	if (!attr->sample_max_stack)
> -- 
> 2.13.6

Re: [PATCH 1/3] perf: Return proper values for user stack errors

[Arnaldo Carvalho de Melo]

Em Mon, Apr 16, 2018 at 01:54:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sun, Apr 15, 2018 at 11:23:50AM +0200, Jiri Olsa escreveu:
> > Return immediately when we find issue in the user
> > stack checks. The error value could get overwritten
> > by following check for PERF_SAMPLE_REGS_INTR.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Also, since you're sending this to syskaller-bugs, was this something
> reported by that tool? If so I think we should communicate with him,
> like described in its docs, no? /me goes to find the doc snippet...

Ok, you did it in 2/3, nevermind then :-)
 
- Arnaldo
> 
> 
> >  kernel/events/core.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index fc1c330c6bd6..4d997bc064d2 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -10191,9 +10191,9 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
> >  		 * __u16 sample size limit.
> >  		 */
> >  		if (attr->sample_stack_user >= USHRT_MAX)
> > -			ret = -EINVAL;
> > +			return -EINVAL;
> >  		else if (!IS_ALIGNED(attr->sample_stack_user, sizeof(u64)))
> > -			ret = -EINVAL;
> > +			return -EINVAL;
> >  	}
> >  
> >  	if (!attr->sample_max_stack)
> > -- 
> > 2.13.6

[tip:perf/urgent] perf: Return proper values for user stack errors

[tip-bot for Jiri Olsa]

Commit-ID:  78b562fbfa2cf0a9fcb23c3154756b690f4905c1
Gitweb:     https://git.kernel.org/tip/78b562fbfa2cf0a9fcb23c3154756b690f4905c1
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Sun, 15 Apr 2018 11:23:50 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 17 Apr 2018 09:47:39 -0300

perf: Return proper values for user stack errors

Return immediately when we find issue in the user stack checks. The
error value could get overwritten by following check for
PERF_SAMPLE_REGS_INTR.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: syzkaller-bugs@googlegroups.com
Cc: x86@kernel.org
Fixes: 60e2364e60e8 ("perf: Add ability to sample machine state on interrupt")
Link: http://lkml.kernel.org/r/20180415092352.12403-1-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 kernel/events/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1bae80aaabfb..67612ce359ad 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10209,9 +10209,9 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 		 * __u16 sample size limit.
 		 */
 		if (attr->sample_stack_user >= USHRT_MAX)
-			ret = -EINVAL;
+			return -EINVAL;
 		else if (!IS_ALIGNED(attr->sample_stack_user, sizeof(u64)))
-			ret = -EINVAL;
+			return -EINVAL;
 	}
 
 	if (!attr->sample_max_stack)

[tip:perf/urgent] perf: Fix sample_max_stack maximum check

[tip-bot for Jiri Olsa]

Commit-ID:  5af44ca53d019de47efe6dbc4003dd518e5197ed
Gitweb:     https://git.kernel.org/tip/5af44ca53d019de47efe6dbc4003dd518e5197ed
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Sun, 15 Apr 2018 11:23:51 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 17 Apr 2018 09:47:40 -0300

perf: Fix sample_max_stack maximum check

The syzbot hit KASAN bug in perf_callchain_store having the entry stored
behind the allocated bounds [1].

We miss the sample_max_stack check for the initial event that allocates
callchain buffers. This missing check allows to create an event with
sample_max_stack value bigger than the global sysctl maximum:

  # sysctl -a | grep perf_event_max_stack
  kernel.perf_event_max_stack = 127

  # perf record -vv -C 1 -e cycles/max-stack=256/ kill
  ...
  perf_event_attr:
    size                             112
    ...
    sample_max_stack                 256
  ------------------------------------------------------------
  sys_perf_event_open: pid -1  cpu 1  group_fd -1  flags 0x8 = 4

Note the '-C 1', which forces perf record to create just single event.
Otherwise it opens event for every cpu, then the sample_max_stack check
fails on the second event and all's fine.

The fix is to run the sample_max_stack check also for the first event
with callchains.

[1] https://marc.info/?l=linux-kernel&m=152352732920874&w=2

Reported-by: syzbot+7c449856228b63ac951e@syzkaller.appspotmail.com
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: syzkaller-bugs@googlegroups.com
Cc: x86@kernel.org
Fixes: 97c79a38cd45 ("perf core: Per event callchain limit")
Link: http://lkml.kernel.org/r/20180415092352.12403-2-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 kernel/events/callchain.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
index 772a43fea825..73cc26e321de 100644
--- a/kernel/events/callchain.c
+++ b/kernel/events/callchain.c
@@ -119,19 +119,22 @@ int get_callchain_buffers(int event_max_stack)
 		goto exit;
 	}
 
+	/*
+	 * If requesting per event more than the global cap,
+	 * return a different error to help userspace figure
+	 * this out.
+	 *
+	 * And also do it here so that we have &callchain_mutex held.
+	 */
+	if (event_max_stack > sysctl_perf_event_max_stack) {
+		err = -EOVERFLOW;
+		goto exit;
+	}
+
 	if (count > 1) {
 		/* If the allocation failed, give up */
 		if (!callchain_cpus_entries)
 			err = -ENOMEM;
-		/*
-		 * If requesting per event more than the global cap,
-		 * return a different error to help userspace figure
-		 * this out.
-		 *
-		 * And also do it here so that we have &callchain_mutex held.
-		 */
-		if (event_max_stack > sysctl_perf_event_max_stack)
-			err = -EOVERFLOW;
 		goto exit;
 	}
 

[tip:perf/urgent] perf: Remove superfluous allocation error check

[tip-bot for Jiri Olsa]

Commit-ID:  bfb3d7b8b906b66551424d7636182126e1d134c8
Gitweb:     https://git.kernel.org/tip/bfb3d7b8b906b66551424d7636182126e1d134c8
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Sun, 15 Apr 2018 11:23:52 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 17 Apr 2018 09:47:40 -0300

perf: Remove superfluous allocation error check

If the get_callchain_buffers fails to allocate the buffer it will
decrease the nr_callchain_events right away.

There's no point of checking the allocation error for
nr_callchain_events > 1. Removing that check.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: syzkaller-bugs@googlegroups.com
Cc: x86@kernel.org
Link: http://lkml.kernel.org/r/20180415092352.12403-3-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 kernel/events/callchain.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
index 73cc26e321de..c187aa3df3c8 100644
--- a/kernel/events/callchain.c
+++ b/kernel/events/callchain.c
@@ -131,14 +131,8 @@ int get_callchain_buffers(int event_max_stack)
 		goto exit;
 	}
 
-	if (count > 1) {
-		/* If the allocation failed, give up */
-		if (!callchain_cpus_entries)
-			err = -ENOMEM;
-		goto exit;
-	}
-
-	err = alloc_callchain_buffers();
+	if (count == 1)
+		err = alloc_callchain_buffers();
 exit:
 	if (err)
 		atomic_dec(&nr_callchain_events);