On Fri, 02 Oct 2026 09:14:31 +0000
[email protected] wrote:
> 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.
Ah, OK. Such usage is not recommended but it can happen and
causes array index overrun.
>
> > + 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?
parse_boot_instance_info() already checks the overlaps.
>
> 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().
Ah, that's a good point! we should check it in
parse_boot_instance_info().
>
> 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.
>
Thank you,
--
Masami Hiramatsu (Google) <[email protected]>
