Message-ID: <ZjqXa25BnFncJmw-@x1>
Date: Tue, 7 May 2024 18:04:43 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Kan Liang <kan.liang@...ux.intel.com>,
	Oliver Upton <oliver.upton@...ux.dev>,
	James Clark <james.clark@....com>,
	Tim Chen <tim.c.chen@...ux.intel.com>,
	Yicong Yang <yangyicong@...ilicon.com>,
	K Prateek Nayak <kprateek.nayak@....com>,
	Yanteng Si <siyanteng@...ngson.cn>,
	Sun Haiyong <sunhaiyong@...ngson.cn>,
	Kajol Jain <kjain@...ux.ibm.com>,
	Ravi Bangoria <ravi.bangoria@....com>, Li Dong <lidong@...o.com>,
	Paran Lee <p4ranlee@...il.com>, Ben Gainey <ben.gainey@....com>,
	Andi Kleen <ak@...ux.intel.com>,
	Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v1 1/8] perf ui browser: Don't save pointer to stack
 memory

On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
> On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
> >
> > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > > > ui_browser__show is capturing the input title that is stack allocated
> > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > > > the string.
> > >
> > > But everything happens in that context, i.e. hist_brower__run() will
> > > call ui_browser__ methods and then exit.
> > >
> > > We end up having browser->title pointing to returned stack memory
> > > (invalid) but there will be no references to it, no?
> > >
> > > If we return to hist_browser__run() we then call ui_browser__show
> > > passing a new title, for "live" stack memory, rinse repeat. Or have you
> > > noticed an actual use-after-"free"?
> >
> > And I'll take the patch, I'm just trying to figure it out if it fixed a
> > real bug or if it just makes the code more future proof, i.e. to avoid
> > us adding code that actually uses invalid stack memory.
> 
> My command line using tui is:
> $ sudo bash -c 'rm /tmp/asan.log*; export
> ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
> sleep 1; /tmp/perf/perf mem report'
> I then go to the perf annotate view and quit. This triggers the asan
> error (from the log file):
> ```

Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
in the full details can hopefully find this message going from the Link:
tag.

- Arnaldo

> ==1254591==ERROR: AddressSanitizer: stack-use-after-return on address
> 0x7f2813331920 at pc 0x7f28180
> 65991 bp 0x7fff0a21c750 sp 0x7fff0a21bf10
> READ of size 80 at 0x7f2813331920 thread T0
>     #0 0x7f2818065990 in __interceptor_strlen
> ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:461
>     #1 0x7f2817698251 in SLsmg_write_wrapped_string
> (/lib/x86_64-linux-gnu/libslang.so.2+0x98251)
>     #2 0x7f28176984b9 in SLsmg_write_nstring
> (/lib/x86_64-linux-gnu/libslang.so.2+0x984b9)
>     #3 0x55c94045b365 in ui_browser__write_nstring ui/browser.c:60
>     #4 0x55c94045c558 in __ui_browser__show_title ui/browser.c:266
>     #5 0x55c94045c776 in ui_browser__show ui/browser.c:288
>     #6 0x55c94045c06d in ui_browser__handle_resize ui/browser.c:206
>     #7 0x55c94047979b in do_annotate ui/browsers/hists.c:2458
>     #8 0x55c94047fb17 in evsel__hists_browse ui/browsers/hists.c:3412
>     #9 0x55c940480a0c in perf_evsel_menu__run ui/browsers/hists.c:3527
>     #10 0x55c940481108 in __evlist__tui_browse_hists ui/browsers/hists.c:3613
>     #11 0x55c9404813f7 in evlist__tui_browse_hists ui/browsers/hists.c:3661
>     #12 0x55c93ffa253f in report__browse_hists tools/perf/builtin-report.c:671
>     #13 0x55c93ffa58ca in __cmd_report tools/perf/builtin-report.c:1141
>     #14 0x55c93ffaf159 in cmd_report tools/perf/builtin-report.c:1805
>     #15 0x55c94000c05c in report_events tools/perf/builtin-mem.c:374
>     #16 0x55c94000d96d in cmd_mem tools/perf/builtin-mem.c:516
>     #17 0x55c9400e44ee in run_builtin tools/perf/perf.c:350
>     #18 0x55c9400e4a5a in handle_internal_command tools/perf/perf.c:403
>     #19 0x55c9400e4e22 in run_argv tools/perf/perf.c:447
>     #20 0x55c9400e53ad in main tools/perf/perf.c:561
>     #21 0x7f28170456c9 in __libc_start_call_main
> ../sysdeps/nptl/libc_start_call_main.h:58
>     #22 0x7f2817045784 in __libc_start_main_impl ../csu/libc-start.c:360
>     #23 0x55c93ff544c0 in _start (/tmp/perf/perf+0x19a4c0) (BuildId:
> 84899b0e8c7d3a3eaa67b2eb35e3d8b2f8cd4c93)
> 
> Address 0x7f2813331920 is located in stack of thread T0 at offset 32 in frame
>     #0 0x55c94046e85e in hist_browser__run ui/browsers/hists.c:746
> 
>   This frame has 1 object(s):
>     [32, 192) 'title' (line 747) <== Memory access at offset 32 is
> inside this variable
> HINT: this may be a false positive if your program uses some custom
> stack unwind mechanism, swapcontext or vfork
> ```
> hist_browser__run isn't on the stack so the asan error looks legit.
> There's no clean init/exit on struct ui_browser so I may be trading a
> use-after-return for a memory leak, but that seems look a good trade
> anyway.
> 
> Thanks,
> Ian
> 
> > - Arnaldo
> >
> > > - Arnaldo
> > >
> > > > Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
> > > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > > > ---
> > > >  tools/perf/ui/browser.c | 4 +++-
> > > >  tools/perf/ui/browser.h | 2 +-
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > > > index 603d11283cbd..c4cdf2ea69b7 100644
> > > > --- a/tools/perf/ui/browser.c
> > > > +++ b/tools/perf/ui/browser.c
> > > > @@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> > > >     mutex_lock(&ui__lock);
> > > >     __ui_browser__show_title(browser, title);
> > > >
> > > > -   browser->title = title;
> > > > +   free(browser->title);
> > > > +   browser->title = strdup(title);
> > > >     zfree(&browser->helpline);
> > > >
> > > >     va_start(ap, helpline);
> > > > @@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
> > > >     mutex_lock(&ui__lock);
> > > >     ui_helpline__pop();
> > > >     zfree(&browser->helpline);
> > > > +   zfree(&browser->title);
> > > >     mutex_unlock(&ui__lock);
> > > >  }
> > > >
> > > > diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
> > > > index 510ce4554050..6e98d5f8f71c 100644
> > > > --- a/tools/perf/ui/browser.h
> > > > +++ b/tools/perf/ui/browser.h
> > > > @@ -21,7 +21,7 @@ struct ui_browser {
> > > >     u8            extra_title_lines;
> > > >     int           current_color;
> > > >     void          *priv;
> > > > -   const char    *title;
> > > > +   char          *title;
> > > >     char          *helpline;
> > > >     const char    *no_samples_msg;
> > > >     void          (*refresh_dimensions)(struct ui_browser *browser);
> > > > --
> > > > 2.45.0.rc1.225.g2a3ae87e7f-goog

Powered by blists - more mailing lists