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

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

[Yifan Zhang]

Traceprobe calculates dynamic string sizes before storing the values.  A
probed pointer can change between those passes, so fetch_store_symstrlen()
can reserve space for a short symbol while fetch_store_symstring() observes
a longer one.  The store path ignores the reserved length and allows the
second symbol to overwrite the trace record.

KASAN reports:

  BUG: KASAN: slab-out-of-bounds in sized_strscpy
  Write of size 8
  sized_strscpy
  module_address_lookup
  __sprint_symbol
  process_fetch_insn

Format the symbol into a temporary KSYM_SYMBOL_LEN buffer with
sprint_symbol(), then copy at most the reserved length minus the trailing
NUL.  Using sprint_symbol() for both sizing and storage preserves identical
symbol formatting while bounding the destination write.

Fixes: b26a124cbfa8 ("tracing/probes: Add symstr type for dynamic events")
Signed-off-by: Yifan Zhang <yfzhang23@stu.pku.edu.cn>
---
Changes in v2:
- use sprint_symbol() for both sizing and storage, with a bounded copy from
  a temporary buffer, instead of formatting the destination with %pS

 kernel/trace/trace_probe_tmpl.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

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;
 }
 
 /* common part of process_fetch_insn*/
-- 
2.39.5 (Apple Git-154)

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

[sashiko-bot]

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