Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] tracing: heap out-of-bounds write in parse_boot_instance_info due to unescaped tabs - [Medium] tracing: duplicate trace array creation via implicit backup lookup causes tracefs init failure --
commit e7d9ac9646a2e104af11665c8c61543d20737c13 Author: Masami Hiramatsu (Google) <[email protected]> tracing: Override the same name trace_instance= by latter one Override the same name boot instance by the latter parameter. This allows user to override the trace instance in bootconfig by the cmdline from bootloader. > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index e4a490d3d08c8..4590d38969450 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -308,6 +309,7 @@ static int __init boot_instance(char *str) > > ret = snprintf(slot, left, "%s\t", str); > boot_instance_index += ret; > + nr_boot_instances++; > > return 1; > } [ ... ] > +__init static void parse_boot_instance_info(struct boot_instance > *boot_instances) > { [ ... ] > + while ((events = strsep(&str, "\t"))) { > + if (!*events) > + continue; [ ... ] > + for (i = 0; i < idx; i++) { > + /* Override by the last defined instance */ > + if (!strcmp(name, boot_instances[i].name)) > + break; > } > > + boot_instances[i].name = name; > + boot_instances[i].flag_delim = flag_delim; > + boot_instances[i].addr_delim = addr_delim; > + boot_instances[i].backup = backup; > + boot_instances[i].events = events; [Severity: High] If a user provides a quoted parameter containing a literal tab (for example, trace_instance="foo\tbar"), won't strsep(&str, "\t") split it into more tokens than what was originally counted in nr_boot_instances? This loop appears to write beyond the bounds of the boot_instances array when there are embedded tabs, which could lead to memory corruption and a kernel panic during early boot. > + if (i == idx) > + idx++; > + } > + > + nr_boot_instances = idx; > +} > + > +__init static void enable_instances(void) > +{ > + struct boot_instance *boot_instances __free(kfree) = NULL; > + struct boot_instance *bi; > + struct trace_array *tr; > + bool memmap_area = false; > + char *tok; > + > + if (!boot_instance_index) > + return; > + > + /* Parse the instance name to check the overlap */ > + boot_instances = kmalloc_array(nr_boot_instances, > + sizeof(*boot_instances), GFP_KERNEL); > + if (!boot_instances) > + return; > + > + parse_boot_instance_info(boot_instances); > + > + for (int i = 0; i < nr_boot_instances; i++) { > + char *rname __free(kfree) = NULL; [ ... ] > - if (trace_array_find(name)) { > - pr_warn("Tracing: Instance %s already exists\n", name); > - continue; > - } > - > - if (backup) { > - if (backup_instance_area(backup, &addr, &size) < 0) > + if (bi->backup) { > + if (backup_instance_area(bi->backup, &addr, &size) < 0) > continue; > } [ ... ] > - tr = trace_array_create_systems(name, NULL, addr, size); > + tr = trace_array_create_systems(bi->name, NULL, addr, size); [Severity: Medium] With the trace_array_find() check removed, does this unconditionally create a new trace array, even if one implicitly exists? If a user specifies a trace_instance with a backup pointing to another instance defined later (e.g., trace_instance=foo=bar trace_instance=bar), backup_instance_area() will implicitly create "bar" via trace_array_get_by_name(). When the loop processes "bar" explicitly later on, it seems this code will call trace_array_create_systems("bar", ...) a second time, duplicating the trace array in ftrace_trace_arrays. This duplicate could cause tracefs_create_dir() to fail and abort initialization of subsequent trace instances. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/179091984881.537422.15308457677039592540.stgit@devnote2?part=1
