* [PATCH v2 RESEND] perf top: Fix rare segfault in thread__comm_len()
@ 2023-03-15 6:42 liuwenyu (D)
2023-03-15 13:30 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 2+ messages in thread
From: liuwenyu (D) @ 2023-03-15 6:42 UTC (permalink / raw)
To: acme@kernel.org >> Arnaldo Carvalho de Melo, mingo,
mark.rutland, alexander.shishkin, jolsa, namhyung, irogers,
brauner, adrian.hunter, linux-perf-users, linux-kernel
Cc: linfeilong, hewenliang (C), yeyunfeng
In thread__comm_len(),strlen() is called outside of the
thread->comm_lock critical section,which may cause a UAF
problems if comm__free() is called by the process_thread
concurrently.
backtrace of the core file is as follows:
(gdb) bt
#0 __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
#1 0x000055ad15d31de5 in thread__comm_len (thread=0x7f627d20e300) at util/thread.c:320
#2 0x000055ad15d4fade in hists__calc_col_len (h=0x7f627d295940, hists=0x55ad1772bfe0)
at util/hist.c:103
#3 hists__calc_col_len (hists=0x55ad1772bfe0, h=0x7f627d295940) at util/hist.c:79
#4 0x000055ad15d52c8c in output_resort (hists=hists@entry=0x55ad1772bfe0, prog=0x0,
use_callchain=false, cb=cb@entry=0x0, cb_arg=0x0) at util/hist.c:1926
#5 0x000055ad15d530a4 in evsel__output_resort_cb (evsel=evsel@entry=0x55ad1772bde0,
prog=prog@entry=0x0, cb=cb@entry=0x0, cb_arg=cb_arg@entry=0x0) at util/hist.c:1945
#6 0x000055ad15d53110 in evsel__output_resort (evsel=evsel@entry=0x55ad1772bde0,
prog=prog@entry=0x0) at util/hist.c:1950
#7 0x000055ad15c6ae9a in perf_top__resort_hists (t=t@entry=0x7ffcd9cbf4f0) at builtin-top.c:311
#8 0x000055ad15c6cc6d in perf_top__print_sym_table (top=0x7ffcd9cbf4f0) at builtin-top.c:346
#9 display_thread (arg=0x7ffcd9cbf4f0) at builtin-top.c:700
#10 0x00007f6282fab4fa in start_thread (arg=<optimized out>) at pthread_create.c:443
#11 0x00007f628302e200 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
The reason is that strlen() get a pointer to a memory that has been freed.
The string pointer is stored in the structure comm_str, which corresponds
to a rb_tree node,when the node is erased, the memory of the string is also freed.
In thread__comm_len(),it gets the pointer within the thread->comm_lock critical section,
but passed to strlen() outside of the thread->comm_lock critical section, and the perf
process_thread may called comm__free() concurrently, cause this segfault problem.
The process is as follows:
display_thread process_thread
-------------- --------------
thread__comm_len
-> thread__comm_str
# held the comm read lock
-> __thread__comm_str(thread)
# release the comm read lock
thread__delete
# held the comm write lock
-> comm__free
-> comm_str__put(comm->comm_str)
-> zfree(&cs->str)
# release the comm write lock
# The memory of the string pointed
to by comm has been free.
-> thread->comm_len = strlen(comm);
This patch expand the critical section range of thread->comm_lock in thread__comm_len(),
to make strlen() called safe.
Signed-off-by: Wenyu Liu <liuwenyu7@huawei.com>
---
v1 -> v2
- format the patch to be applied
tools/perf/util/thread.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index e3e5427e1c3c..a2490a20eb56 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -311,17 +311,30 @@ const char *thread__comm_str(struct thread *thread)
return str;
}
+static int __thread__comm_len(struct thread *thread, const char *comm)
+{
+ if (!comm)
+ return 0;
+ thread->comm_len = strlen(comm);
+
+ return thread->comm_len;
+}
+
/* CHECKME: it should probably better return the max comm len from its comm list */
int thread__comm_len(struct thread *thread)
{
- if (!thread->comm_len) {
- const char *comm = thread__comm_str(thread);
- if (!comm)
- return 0;
- thread->comm_len = strlen(comm);
+ int comm_len = thread->comm_len;
+
+ if (!comm_len) {
+ const char *comm;
+
+ down_read(&thread->comm_lock);
+ comm = __thread__comm_str(thread);
+ comm_len = __thread__comm_len(thread, comm);
+ up_read(&thread->comm_lock);
}
- return thread->comm_len;
+ return comm_len;
}
size_t thread__fprintf(struct thread *thread, FILE *fp)
--
2.36.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2 RESEND] perf top: Fix rare segfault in thread__comm_len()
2023-03-15 6:42 [PATCH v2 RESEND] perf top: Fix rare segfault in thread__comm_len() liuwenyu (D)
@ 2023-03-15 13:30 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 2+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-15 13:30 UTC (permalink / raw)
To: liuwenyu (D)
Cc: mingo, mark.rutland, alexander.shishkin, jolsa, namhyung, irogers,
brauner, adrian.hunter, linux-perf-users, linux-kernel,
linfeilong, hewenliang (C), yeyunfeng
Em Wed, Mar 15, 2023 at 02:42:17PM +0800, liuwenyu (D) escreveu:
> In thread__comm_len(),strlen() is called outside of the
> thread->comm_lock critical section,which may cause a UAF
> problems if comm__free() is called by the process_thread
> concurrently.
Applied, next time you resend a patch just to fix formatting, without
changing what the patch does, please collect tags provided for the
previous version, like the one from Namhyung:
Acked-by: Namhyung Kim <namhyung@kernel.org>
I did it because I saw it for v1, so added now.
Thanks,
- Arnaldo
> backtrace of the core file is as follows:
>
> (gdb) bt
> #0 __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
> #1 0x000055ad15d31de5 in thread__comm_len (thread=0x7f627d20e300) at util/thread.c:320
> #2 0x000055ad15d4fade in hists__calc_col_len (h=0x7f627d295940, hists=0x55ad1772bfe0)
> at util/hist.c:103
> #3 hists__calc_col_len (hists=0x55ad1772bfe0, h=0x7f627d295940) at util/hist.c:79
> #4 0x000055ad15d52c8c in output_resort (hists=hists@entry=0x55ad1772bfe0, prog=0x0,
> use_callchain=false, cb=cb@entry=0x0, cb_arg=0x0) at util/hist.c:1926
> #5 0x000055ad15d530a4 in evsel__output_resort_cb (evsel=evsel@entry=0x55ad1772bde0,
> prog=prog@entry=0x0, cb=cb@entry=0x0, cb_arg=cb_arg@entry=0x0) at util/hist.c:1945
> #6 0x000055ad15d53110 in evsel__output_resort (evsel=evsel@entry=0x55ad1772bde0,
> prog=prog@entry=0x0) at util/hist.c:1950
> #7 0x000055ad15c6ae9a in perf_top__resort_hists (t=t@entry=0x7ffcd9cbf4f0) at builtin-top.c:311
> #8 0x000055ad15c6cc6d in perf_top__print_sym_table (top=0x7ffcd9cbf4f0) at builtin-top.c:346
> #9 display_thread (arg=0x7ffcd9cbf4f0) at builtin-top.c:700
> #10 0x00007f6282fab4fa in start_thread (arg=<optimized out>) at pthread_create.c:443
> #11 0x00007f628302e200 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
>
> The reason is that strlen() get a pointer to a memory that has been freed.
>
> The string pointer is stored in the structure comm_str, which corresponds
> to a rb_tree node,when the node is erased, the memory of the string is also freed.
>
> In thread__comm_len(),it gets the pointer within the thread->comm_lock critical section,
> but passed to strlen() outside of the thread->comm_lock critical section, and the perf
> process_thread may called comm__free() concurrently, cause this segfault problem.
>
> The process is as follows:
>
> display_thread process_thread
> -------------- --------------
>
> thread__comm_len
> -> thread__comm_str
> # held the comm read lock
> -> __thread__comm_str(thread)
> # release the comm read lock
> thread__delete
> # held the comm write lock
> -> comm__free
> -> comm_str__put(comm->comm_str)
> -> zfree(&cs->str)
> # release the comm write lock
> # The memory of the string pointed
> to by comm has been free.
> -> thread->comm_len = strlen(comm);
>
> This patch expand the critical section range of thread->comm_lock in thread__comm_len(),
> to make strlen() called safe.
>
> Signed-off-by: Wenyu Liu <liuwenyu7@huawei.com>
> ---
> v1 -> v2
> - format the patch to be applied
>
> tools/perf/util/thread.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index e3e5427e1c3c..a2490a20eb56 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -311,17 +311,30 @@ const char *thread__comm_str(struct thread *thread)
> return str;
> }
>
> +static int __thread__comm_len(struct thread *thread, const char *comm)
> +{
> + if (!comm)
> + return 0;
> + thread->comm_len = strlen(comm);
> +
> + return thread->comm_len;
> +}
> +
> /* CHECKME: it should probably better return the max comm len from its comm list */
> int thread__comm_len(struct thread *thread)
> {
> - if (!thread->comm_len) {
> - const char *comm = thread__comm_str(thread);
> - if (!comm)
> - return 0;
> - thread->comm_len = strlen(comm);
> + int comm_len = thread->comm_len;
> +
> + if (!comm_len) {
> + const char *comm;
> +
> + down_read(&thread->comm_lock);
> + comm = __thread__comm_str(thread);
> + comm_len = __thread__comm_len(thread, comm);
> + up_read(&thread->comm_lock);
> }
>
> - return thread->comm_len;
> + return comm_len;
> }
>
> size_t thread__fprintf(struct thread *thread, FILE *fp)
> --
> 2.36.1
--
- Arnaldo
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-03-15 13:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-15 6:42 [PATCH v2 RESEND] perf top: Fix rare segfault in thread__comm_len() liuwenyu (D)
2023-03-15 13:30 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).