Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Error unwinding in event_hist_trigger_parse() causes Use-After-Free and Double-Free of hist_data, and leaves dangling trigger_data on the global named_triggers list. - [High] Use-after-free of the compatible hist_data when removing a target histogram that uses field variables. -- commit a635652ad2d37c0cc5b35a037b2c68349bec678b Author: Donggeun Yoo <[email protected]> tracing: Unregister field variable histograms when the command fails An action that names a field on another event needs that field as a variable, so create_field_var_hist() registers a second hist trigger on that event to provide it, and records it in target_hist_data->field_var_hists[]. If a later part of the same command fails, create_actions() returns an error and event_hist_trigger_parse() jumps to out_free. Closes: https://lore.kernel.org/all/[email protected]/ > diff --git a/kernel/trace/trace_events_hist.c > b/kernel/trace/trace_events_hist.c > index 963e0d6b61fdf..8e6c699b75c2a 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -3138,20 +3138,18 @@ create_field_var_hist(struct hist_trigger_data > *target_hist_data, > > kfree(cmd); > > + n = target_hist_data->n_field_var_hists; > + target_hist_data->field_var_hists[n] = var_hist; > + target_hist_data->n_field_var_hists++; > + > /* If we can't find the variable, something went wrong */ > event_var = find_synthetic_field_var(target_hist_data, subsys_name, > event_name, field_name); > if (IS_ERR_OR_NULL(event_var)) { > - kfree(var_hist->cmd); > - kfree(var_hist); > hist_err(tr, HIST_ERR_SYNTH_VAR_NOT_FOUND, errpos(field_name)); > return ERR_PTR(-EINVAL); > } > > - n = target_hist_data->n_field_var_hists; > - target_hist_data->field_var_hists[n] = var_hist; > - target_hist_data->n_field_var_hists++; > - > return event_var; > } [Severity: High] This is a pre-existing issue, but can this lead to a use-after-free of the compatible histogram? Earlier in create_field_var_hist(), find_compatible_hist() finds an existing histogram to copy its keys and filter, and stores it in var_hist->hist_data: kernel/trace/trace_events_hist.c:create_field_var_hist() { ... hist_data = find_compatible_hist(target_hist_data, file); ... var_hist->hist_data = hist_data; ... } Since the original compatible histogram has no variables attached to it, check_var_refs() will return false, allowing a user to safely remove it while the target histogram is still active. When the compatible histogram is removed, its hist_data is freed via destroy_hist_data(). Later, when the target histogram is removed, unregister_field_var_hists() is called: kernel/trace/trace_events_hist.c:unregister_field_var_hists() { ... file = hist_data->field_var_hists[i]->hist_data->event_file; ... } Would this dereference the freed compatible hist_data, resulting in a use-after-free? > @@ -6976,6 +6974,8 @@ static int event_hist_trigger_parse(struct > event_command *cmd_ops, > > trigger_data_free(trigger_data); > > + unregister_field_var_hists(hist_data); > + > destroy_hist_data(hist_data); > goto out; > } [Severity: High] This is a pre-existing issue, but can this error path lead to a double-free and use-after-free if named trigger registration fails? If a named trigger is successfully linked to existing state in hist_register_trigger(), save_named_trigger() is called which adds trigger_data to the global named_triggers list, and the local hist_data object is freed: kernel/trace/trace_events_hist.c:hist_register_trigger() { ... if (named_data) { remove_hist_vars(hist_data); destroy_hist_data(hist_data); } ... } If a subsequent initialization step fails in hist_register_trigger() (such as tracing_set_clock() failing), it returns an error without unwinding the list addition. When this happens, event_hist_trigger_parse() jumps to out_free and calls trigger_data_free(trigger_data). Does this leave a dangling pointer in the global named_triggers list? Additionally, this path unconditionally calls destroy_hist_data(hist_data). If hist_data was already freed by hist_register_trigger(), could this result in a double-free? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
