perf: Return proper values for user stack errors
Jiri Olsa
reviewerPATCH 1/3 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.
--- kernel/events/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/events/core.c +2 -2
@@ -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)
Signature · 1 line
2.13.6
Jiri Olsa
reviewerPATCH 2/3 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 = 4Note 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
--- kernel/events/callchain.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
kernel/events/callchain.c +12 -9
@@ -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; }
Signature · 1 line
2.13.6
tip-bot for Jiri Olsa
merge bot[tip:perf/urgent] perf: Fix sample_max_stack maximum check
perf: Fix sample_max_stack maximum check
Commit-ID: 5af44ca53d019de47efe6dbc4003dd518e5197ed Gitweb: https://git.kernel.org/tip/5af44ca53d019de47efe6dbc4003dd518e5197ed Author: Jiri Olsa <[email protected]> AuthorDate: Sun, 15 Apr 2018 11:23:51 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> 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 = 4Note 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
--- kernel/events/callchain.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
kernel/events/callchain.c +12 -9
@@ -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; }
Jiri Olsa
reviewerPATCH 3/3 perf: Remove superfluous allocation error check
In reply to Jiri OlsaIf 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.
--- kernel/events/callchain.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
kernel/events/callchain.c +2 -8
@@ -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);
Signature · 1 line
2.13.6
tip-bot for Jiri Olsa
merge bot[tip:perf/urgent] perf: Remove superfluous allocation error check
perf: Remove superfluous allocation error check
Commit-ID: bfb3d7b8b906b66551424d7636182126e1d134c8 Gitweb: https://git.kernel.org/tip/bfb3d7b8b906b66551424d7636182126e1d134c8 Author: Jiri Olsa <[email protected]> AuthorDate: Sun, 15 Apr 2018 11:23:52 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> 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.
--- kernel/events/callchain.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
kernel/events/callchain.c +2 -8
@@ -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);
Em Sun, Apr 15, 2018 at 11:23:50AM +0200, Jiri Olsa escreveu:
Quoted 3 lines
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:
Quoted 23 lines
Signed-off-by Jiri Olsa <[email protected]>--- kernel/events/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)kernel/events/core.c +2
-2@@ -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)Signature · 1 line
2.13.6
Arnaldo Carvalho de Melo
maintainerEm Sun, Apr 15, 2018 at 11:23:50AM +0200, Jiri Olsa escreveu:
Quoted 5 lines
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 <[email protected]>
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...
Quoted 21 lines
kernel/events/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)kernel/events/core.c +2
-2@@ -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)Signature · 1 line
2.13.6
Arnaldo Carvalho de Melo
maintainerEm Mon, Apr 16, 2018 at 01:54:30PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 10 lines
Em Sun, Apr 15, 2018 at 11:23:50AM +0200, Jiri Olsa escreveu:
Quoted 5 lines
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 <[email protected]>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
Quoted 23 lines
Quoted 23 lines
kernel/events/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)kernel/events/core.c +2
-2@@ -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)Signature · 1 line
2.13.6
tip-bot for Jiri Olsa
merge botperf: Return proper values for user stack errors
Commit-ID: 78b562fbfa2cf0a9fcb23c3154756b690f4905c1 Gitweb: https://git.kernel.org/tip/78b562fbfa2cf0a9fcb23c3154756b690f4905c1 Author: Jiri Olsa <[email protected]> AuthorDate: Sun, 15 Apr 2018 11:23:50 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> 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.
--- kernel/events/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/events/core.c +2 -2
@@ -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)