Re: [PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Stack buffer overflow in `actions` and `options` arrays due to unbounded option additions.
--

--- Patch [2]: [PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action ---
commit 72bd59b6c2f43a82a793219b81133029ab0f3b61
Author: Ian Rogers <irogers@google.com>

perf ui hists: Remove duplicated thread in popup_action

This commit removes the thread field from struct popup_action as it is
duplicated in the map_symbol structure. This ensures there is only ever
one thread associated with a popup_action.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 6353be9dba865..a93846b8dc885 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c

[ ... ]

> @@ -3305,7 +3304,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  				 */
>  				do_zoom_dso(browser, actions);
>  			} else if (top == &browser->hists->thread_filter) {
> -				actions->thread = thread;
> +				actions->ms.thread = thread;
>  			} else if (top == &browser->hists->socket_filter) {
>  				do_zoom_socket(browser, actions);

[Severity: High]
This isn't a bug introduced by this patch, but looking at how the actions
array is populated in evsel__hists_browse(), is there a potential stack buffer
overflow here?

The options and actions arrays are statically sized to 16 elements on the
stack:

    #define MAX_OPTIONS  16
    char *options[MAX_OPTIONS];
    struct popup_action actions[MAX_OPTIONS];

Later in the same function, under the skip_annotation label, the code
unconditionally appends menu options using a sequence of calls like:

    nr_options += add_annotate_type_opt(&actions[nr_options], ...);
    nr_options += add_thread_opt(browser, &actions[nr_options], ...);
    nr_options += add_dso_opt(browser, &actions[nr_options], ...);

Depending on the flags and data available in the perf.data file (such as
data type annotations, threads, callchains, and time sorting), couldn't this
sequence of additions append up to 19 options, exceeding the 16-element limit?

Because the options and actions arrays are allocated adjacently on the stack,
could writing an asprintf string pointer past the end of the options array
directly overwrite the fn function pointer of actions[0]? 

If the user then selects the first menu option, could this result in a control
flow hijack when the program dereferences and jumps to the heap string pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709165220.1411149-1-irogers@google.com?part=2