All of lore.kernel.org
 help / color / mirror / Atom feed
* [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
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 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.