Re: [PATCH v2] tracing/probes: bound symstr stores to reserved length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tracing/probes: missing __data_loc length update in fetch_store_symstring()
--

commit 2b3ccf8dccd0967bb835452c02ebe5b581c10ea8
Author: Yifan Zhang <yfzhang23@stu.pku.edu.cn>

tracing/probes: bound symstr stores to reserved length

This commit bounds symbol string writes in fetch_store_symstring() to the
reserved buffer length by formatting into a stack buffer with sprint_symbol()
and copying at most maxlen - 1 bytes into the trace record.

> diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
> index 8db12f758fdae..dea6772652550 100644
> --- a/kernel/trace/trace_probe_tmpl.h
> +++ b/kernel/trace/trace_probe_tmpl.h
> @@ -87,15 +87,24 @@ fetch_store_symstrlen(unsigned long addr)
>  static nokprobe_inline int
>  fetch_store_symstring(unsigned long addr, void *dest, void *base)
>  {
> +	char namebuf[KSYM_SYMBOL_LEN];
>  	int maxlen = get_loc_len(*(u32 *)dest);
>  	void *__dest;
> +	int len;
>  
>  	if (unlikely(!maxlen))
>  		return -ENOMEM;
>  
>  	__dest = get_loc_data(dest, base);
> +	len = sprint_symbol(namebuf, addr);
> +	if (len < 0)
> +		return 0;
> +
> +	len = min(len, maxlen - 1);
> +	memcpy(__dest, namebuf, len);
> +	((char *)__dest)[len] = '\0';
>  
> -	return sprint_symbol(__dest, addr);
> +	return len + 1;

[Severity: High]
Does fetch_store_symstring() also need to update *(u32 *)dest with the actual
stored length via make_data_loc() (or set_data_loc()), similar to
fetch_store_string() and fetch_store_string_user() in
kernel/trace/trace_probe_kernel.h?

Before calling process_fetch_insn(), store_trace_args() in
kernel/trace/trace_probe_tmpl.h initializes the __data_loc descriptor to the
entire remaining dynamic buffer capacity across all remaining dynamic
arguments:

	if (unlikely(arg->dynamic))
		*dl = make_data_loc(maxlen, dyndata - base);
	ret = process_fetch_insn(arg->code, rec, edata, dl, base);
	if (arg->dynamic && likely(ret > 0)) {
		dyndata += ret;
		maxlen -= ret;
	}

Because fetch_store_symstring() writes len + 1 bytes and returns len + 1
without updating *(u32 *)dest, the upper 16 bits of the __data_loc field
remain set to maxlen whenever a symstr argument is followed by another
dynamic argument, or when the probed pointer changes between
fetch_store_symstrlen() and fetch_store_symstring() to a shorter symbol so
that len + 1 < maxlen.

Downstream consumers that extract the string length from the upper 16 bits of
the __data_loc descriptor will then read maxlen bytes instead of len + 1
bytes, spanning into subsequent dynamic arguments or uninitialized ring-buffer
tail bytes:

- filter_pred_strloc() in kernel/trace/trace_events_filter.c passes
  str_len = str_item >> 16 to pred->regex->match(), breaking suffix and glob
  matching on symstr fields.
- add_to_key() in kernel/trace/trace_events_hist.c copies
  size = *(u32 *)(rec + field->offset) >> 16 bytes into the histogram
  compound key.
- print_fields() in kernel/trace/trace_output.c reads len = *(int *)pos >> 16
  bytes from the event record.

Similarly, on the len < 0 path, returning 0 without updating *(u32 *)dest to a
0-length data_loc leaves get_loc_len(*(u32 *)dest) == maxlen > 0 while __dest
is uninitialized, causing PRINT_TYPE_FUNC_NAME(string)() in
kernel/trace/trace_probe.c to format uninitialized ring-buffer memory instead
of printing FAULT_STRING.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179367391951.8187.17152618045554425533@stu.pku.edu.cn?part=1