Re: [PATCH v2 1/2] tracing: Override the same name trace_instance= by latter one

[sashiko-bot@kernel.org]

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) <mhiramat@kernel.org>

    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