tracing: Updates for 7.4
Subsystems (3)
cover tracing: Updates for 7.4
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace/for-next
Head SHA1: f86641ffea95ee6f62d829929a69be7ab1496ce5
Adrián García Casado (1):
tracing/osnoise: Add __printf attribute to osnoise_print()
David Carlier (2):
tracing: Hold trace_types_lock when updating modules in persistent instances
tracing: Hold trace_types_lock when resetting last boot data from readers
Donggeun Yoo (1):
tracing: Save the event file instead of the compatible histogram
Haoran Wang (1):
tracing/syscalls: Use copy_from_user for the faultable syscall buffer
Lee Jones (1):
tracing/histograms: Verify event_call identity when updating synth_event refcount
----
kernel/trace/trace.c | 21 ++++++++++++++++++--- kernel/trace/trace_events_hist.c | 19 +++++++------------ kernel/trace/trace_events_synth.c | 10 ++++++++++ kernel/trace/trace_osnoise.c | 1 + kernel/trace/trace_synth.h | 1 + kernel/trace/trace_syscalls.c | 2 +- 6 files changed, 38 insertions(+), 16 deletions(-)
PATCH 1/6 tracing/syscalls: Use copy_from_user for the faultable syscall buffer
From: Haoran Wang <[email protected]>
syscall_copy_user_array() copies the buffer argument of faultable syscalls (write, pwrite64, ...) with __copy_from_user(), which skips access_ok(). Use copy_from_user() instead so a non-user address is rejected and the entry is recorded as faulted.
It could be a security concern for machines that trust perf a bit too much.
--- kernel/trace/trace_syscalls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace_syscalls.c +1 -1
@@ -683,7 +683,7 @@ static int syscall_copy_user_array(char *buf, const char __user *ptr,for (int i =0 ; i < args->uargs; i++, buf +=SYSCALL_FAULT_ARG_SZ ) { ptr = (char __user *)args->ptr_array[i]; - ret =__copy_from_user (buf, ptr, size); + ret =copy_from_user (buf, ptr, size); args->read[i] = ret ?-1 : size; }return 0 ;
Signature · 1 line
2.53.0
PATCH 2/6 tracing: Hold trace_types_lock when updating modules in persistent instances
In reply to Steven RostedtFrom: David Carlier <[email protected]>
trace_module_record() walks ftrace_trace_arrays from the module notifier without trace_types_lock. A concurrent rmdir of an instance removes it from the list and frees it under that lock, so a module load or unload can walk into a freed trace_array.
--- kernel/trace/trace.c | 2 ++ 1 file changed, 2 insertions(+)
kernel/trace/trace.c +2 -0
@@ -9244,6 +9244,8 @@ static void trace_module_record(struct module *mod, bool add)struct trace_array *tr;unsigned long flags; +guard (mutex)(&trace_types_lock); +list_for_each_entry (tr, &ftrace_trace_arrays, list) { flags = tr->flags & (TRACE_ARRAY_FL_BOOT |TRACE_ARRAY_FL_LAST_BOOT );/* Update any persistent trace array that has already been started */
Signature · 1 line
2.53.0
PATCH 3/6 tracing: Hold trace_types_lock when resetting last boot data from readers
In reply to Steven RostedtFrom: David Carlier <[email protected]>
update_last_data_if_empty() is called from tracing_open() with O_TRUNC, tracing_read_pipe() and tracing_buffers_read() without trace_types_lock. Two readers can both pass the LAST_BOOT test and free module_delta twice, and a module notifier can update module_delta while it is being freed.
Take trace_types_lock in those paths and keep the callers that already hold it on an unlocked variant.
--- kernel/trace/trace.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
kernel/trace/trace.c +16 -3
@@ -3603,6 +3603,7 @@ static int tracing_single_release_tr(struct inode *inode, struct file *file)return single_release (inode, file); } +static bool __update_last_data_if_empty (struct trace_array *tr);static bool update_last_data_if_empty (struct trace_array *tr);static int tracing_open (struct inode *inode,struct file *file)
@@ -4697,7 +4698,7 @@ tracing_set_trace_read(struct file *filp, char __user *ubuf,int tracer_init (struct tracer *t,struct trace_array *tr) {tracing_reset_online_cpus (&tr->array_buffer); -update_last_data_if_empty (tr); +__update_last_data_if_empty (tr);return t->init (tr); }
@@ -5371,8 +5372,10 @@ static int tracing_wait_pipe(struct file *filp)return 1 ; } -static bool update_last_data_if_empty (struct trace_array *tr) +static bool __update_last_data_if_empty (struct trace_array *tr) { +lockdep_assert_held (&trace_types_lock); +if (!(tr->flags &TRACE_ARRAY_FL_LAST_BOOT ))return false;
@@ -5387,6 +5390,16 @@ static bool update_last_data_if_empty(struct trace_array *tr)return true; } +static bool update_last_data_if_empty (struct trace_array *tr) +{ +/* Avoid the lock on the read path unless this is a last boot buffer */ +if (!(tr->flags &TRACE_ARRAY_FL_LAST_BOOT )) +return false; + +guard (mutex)(&trace_types_lock); +return __update_last_data_if_empty (tr); +} +/* * Consumer reader. */
@@ -6476,7 +6489,7 @@ int tracing_set_clock(struct trace_array *tr, const char *clockstr)ring_buffer_set_clock (tr->snapshot_buffer.buffer, trace_clocks[i].func);tracing_reset_online_cpus (&tr->snapshot_buffer);#endif -update_last_data_if_empty (tr); +__update_last_data_if_empty (tr);if (tr->scratch && !(tr->flags &TRACE_ARRAY_FL_LAST_BOOT )) {struct trace_scratch *tscratch = tr->scratch;
Signature · 1 line
2.53.0
PATCH 4/6 tracing: Save the event file instead of the compatible histogram
In reply to Steven RostedtFrom: Donggeun Yoo <[email protected]>
create_field_var_hist() creates a histogram on the matched event to supply a field variable, and records it on the target so it can be torn down later:
hist_data = find_compatible_hist(target_hist_data, file); ... /* Save the compatible histogram information */ var_hist->hist_data = hist_data;
hist_data is not the histogram it creates. It is one that was already there, whose keys the new one copies. The saved pointer has a single user, unregister_field_var_hists(), which runs when the target is removed:
file = hist_data->field_var_hists[i]->hist_data->event_file;
find_compatible_hist() matches on keys alone, so it can pick one with no variables of its own. check_var_refs() then returns false and the user can remove it while the target still points at it:
BUG: KASAN: slab-use-after-free in event_hist_trigger_free+0x2b2/0x320 Read of size 8 at addr ffff8880093b90e0 by task init/1 Freed by task 1: kfree+0x154/0x420 event_hist_trigger_free+0x1d5/0x320 event_hist_trigger_parse+0x35f3/0x69e0 trigger_process_regex+0x1a6/0x250
Save the event file instead. It is all the dereference ever wanted, and it does not go away when the user removes a trigger.
--- kernel/trace/trace_events_hist.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
kernel/trace/trace_events_hist.c +4 -4
@@ -557,7 +557,7 @@ struct field_var { };struct field_var_hist { -struct hist_trigger_data *hist_data; +struct trace_event_file *file;char *cmd; };
@@ -3129,8 +3129,8 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,return ERR_PTR (-ENOMEM ); } -/* Save the compatible histogram information */ - var_hist->hist_data = hist_data; +/* Needed to remove the histogram when the target goes away */ + var_hist->file = file;/* Create the new histogram with our variable */ ret = event_hist_trigger_parse(&trigger_hist_cmd, file,
@@ -6362,7 +6362,7 @@ static void unregister_field_var_hists(struct hist_trigger_data *hist_data)int ret;for (i =0 ; i < hist_data->n_field_var_hists; i++) { - file = hist_data->field_var_hists[i]->hist_data->event_file; + file = hist_data->field_var_hists[i]->file; cmd = hist_data->field_var_hists[i]->cmd; ret =event_hist_trigger_parse (&trigger_hist_cmd, file,"!hist" ,"hist" , cmd);
Signature · 1 line
2.53.0
PATCH 5/6 tracing/histograms: Verify event_call identity when updating synth_event refcount
In reply to Steven RostedtFrom: Lee Jones <[email protected]>
In event_hist_trigger_parse() and hist_unreg_all(), the histogram registration and unregistration paths look up find_synth_event() using trace_event_name(file->event_call) and adjust se->ref if a match is found.
Because find_synth_event() only compares the event basename, registering or removing a histogram on a non-synthetic trace event whose basename matches a synthetic event erroneously modifies the synthetic event's reference count. If a histogram is registered on the non-synthetic event before the synthetic event is created and removed afterwards, se->ref is decremented without a matching increment, allowing the synthetic event to be freed while an active histogram action still holds a pointer to it.
Introduce find_synth_event_call() to verify that &se->call matches file->event_call before adjusting se->ref.
--- kernel/trace/trace_events_hist.c | 11 +++-------- kernel/trace/trace_events_synth.c | 10 ++++++++++ kernel/trace/trace_synth.h | 1 + 3 files changed, 14 insertions(+), 8 deletions(-)
kernel/trace/trace_events_hist.c +3 -8
@@ -6812,7 +6812,6 @@ static void hist_unreg_all(struct trace_event_file *file)struct event_trigger_data *test, *n;struct hist_trigger_data *hist_data;struct synth_event *se; -const char *se_name;lockdep_assert_held (&event_mutex);
@@ -6825,8 +6824,7 @@ static void hist_unreg_all(struct trace_event_file *file)list_del_rcu (&test->list);trace_event_trigger_enable_disable (file,0 ); - se_name =trace_event_name (file->event_call); - se =find_synth_event (se_name); + se =find_synth_event_call (file->event_call);if (se) se->ref--;
@@ -6850,7 +6848,6 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,struct hist_trigger_data *hist_data;char *param, *filter, *p, *start;struct synth_event *se; -const char *se_name;bool remove;int ret =0 ;
@@ -6944,8 +6941,7 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops, }event_trigger_unregister (cmd_ops, file, glob+1 , trigger_data); - se_name =trace_event_name (file->event_call); - se =find_synth_event (se_name); + se =find_synth_event_call (file->event_call);if (se) se->ref--; ret =0 ;
@@ -6980,8 +6976,7 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,if (ret) goto out_unreg; - se_name =trace_event_name (file->event_call); - se =find_synth_event (se_name); + se =find_synth_event_call (file->event_call);if (se) se->ref++; out:
kernel/trace/trace_events_synth.c +10 -0
@@ -914,6 +914,16 @@ struct synth_event *find_synth_event(const char *name)return NULL ; } +struct synth_event *find_synth_event_call (struct trace_event_call *call) +{ +struct synth_event *se; +const char *name; + + name =trace_event_name (call); + se =find_synth_event (name); +return se && &se->call == call ? se :NULL ; +} +static struct trace_event_fields synth_event_fields_array[] = { { .type =TRACE_FUNCTION_TYPE , .define_fields = synth_event_define_fields },
kernel/trace/trace_synth.h +1 -0
@@ -37,5 +37,6 @@ struct synth_event { };extern struct synth_event *find_synth_event (const char *name); +extern struct synth_event *find_synth_event_call (struct trace_event_call *call);#endif /* __TRACE_SYNTH_H */
Signature · 1 line
2.53.0
PATCH 6/6 tracing/osnoise: Add __printf attribute to osnoise_print()
In reply to Steven RostedtBuilding kernel/trace/trace_osnoise.o with W=1 triggers the following compiler warning:
kernel/trace/trace_osnoise.c:96:17: warning: function 'osnoise_print' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
osnoise_print() forwards its format string and va_list arguments to trace_array_vprintk(), which is annotated with __printf(3, 0).
Add the __printf(1, 2) attribute to osnoise_print() to silence the W=1 warning and enable compile-time format string checking for its callers.
--- kernel/trace/trace_osnoise.c | 1 + 1 file changed, 1 insertion(+)
kernel/trace/trace_osnoise.c +1 -0
@@ -83,6 +83,7 @@ struct osnoise_instance {static struct list_head osnoise_instances; +__printf(1, 2)static void osnoise_print (const char *fmt, ...) {struct osnoise_instance *inst;
Signature · 1 line
2.53.0