Messages in this thread | | | Date | Tue, 7 Mar 2023 11:35:16 +0800 | Subject | Re: [PATCH] perf top: Fix rare segfault in thread__comm_len() | From | "liuwenyu (D)" <> |
| |
Sorry, the patch was sent in an incorrect format due to my email client configuration. I'll resubmit a v2 version patch soon.
在 2023/3/7 4:28, Arnaldo Carvalho de Melo 写道: > Em Fri, Mar 03, 2023 at 06:02: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. >> >> 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. > > The patch isn't applying, please check this and resubmit, the first > issue is that it is malformed at that CHECKME comment, that is broken in > two lines, I fixed that and tried again, still doesn't apply: > > ⬢[acme@toolbox perf-tools]$ patch -p1 < ./20230303_liuwenyu7_perf_top_fix_rare_segfault_in_thread__comm_len.mbx > patching file tools/perf/util/thread.c > Hunk #1 FAILED at 311. > 1 out of 1 hunk FAILED -- saving rejects to file tools/perf/util/thread.c.rej > ⬢[acme@toolbox perf-tools]$ cat tools/perf/util/thread.c.rej > --- tools/perf/util/thread.c > +++ 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) > ⬢[acme@toolbox perf-tools]$ > > > >> Signed-off-by: Wenyu Liu <liuwenyu7@huawei.com> >> --- >> 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 >> >
| |