Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] tracing/probes: symbol format mismatch in fetch_store_symstring() --
commit 1dd7922bca1f3057be72658bb3bb50c1a3ed082f Author: Yifan Zhang <[email protected]> tracing/probes: bound symstr stores to reserved length This commit bounds symbol string formatting in fetch_store_symstring() to the length reserved during the sizing pass by replacing sprint_symbol() with scnprintf(..., "%pS", ...), preventing out-of-bounds writes when a probed pointer changes between passes. > diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h > index 8db12f758fdae..cc317cb444e43 100644 > --- a/kernel/trace/trace_probe_tmpl.h > +++ b/kernel/trace/trace_probe_tmpl.h > @@ -95,7 +95,7 @@ fetch_store_symstring(unsigned long addr, void *dest, void > *base) > > __dest = get_loc_data(dest, base); > > - return sprint_symbol(__dest, addr); > + return scnprintf(__dest, maxlen, "%pS", (void *)addr) + 1; [Severity: Medium] Can switching fetch_store_symstring() to "%pS" while leaving fetch_store_symstrlen() on sprint_symbol() cause a size mismatch between the reservation pass and the store pass even when addr does not change? In kernel/trace/trace_probe_tmpl.h, __get_data_size() -> process_fetch_insn() -> fetch_store_symstrlen() still computes the required buffer size with sprint_symbol(): kernel/trace/trace_probe_tmpl.h:fetch_store_symstrlen() { char namebuf[KSYM_SYMBOL_LEN]; int ret; ret = sprint_symbol(namebuf, addr); if (ret < 0) return 0; return ret + 1; } However, in lib/vsprintf.c, pointer() -> symbol_string() handles "%pS" differently from a direct sprint_symbol() call in two ways: lib/vsprintf.c:pointer() { switch (*fmt) { case 'S': case 's': ptr = dereference_symbol_descriptor(ptr); fallthrough; case 'B': return symbol_string(buf, end, ptr, spec, fmt); } lib/vsprintf.c:symbol_string() { #ifdef CONFIG_KALLSYMS ... else if (*fmt != 's') sprint_symbol(sym, value); ... return string_nocheck(buf, end, sym, spec); #else return special_hex_number(buf, end, value, sizeof(void *)); #endif } On architectures with CONFIG_HAVE_FUNCTION_DESCRIPTORS=y, sprint_symbol() in fetch_store_symstrlen() inspects the raw descriptor address (for example, formatting a short hex address when CONFIG_KALLSYMS_ALL is disabled), whereas "%pS" in fetch_store_symstring() dereferences the descriptor to the target function symbol, which can be longer than the reserved length. Similarly, when CONFIG_KALLSYMS is disabled (such as with CONFIG_EPROBE_EVENTS=y), sprint_symbol() writes an empty string and fetch_store_symstrlen() reserves 1 byte, whereas "%pS" formats the full hex address via special_hex_number(). When the formatted "%pS" string is longer than the sprint_symbol() result, scnprintf() either truncates the symstr field or, if another dynamic argument (such as string, ustring, or symstr) follows in the same probe event, consumes the remaining shared dynamic buffer maxlen passed by store_trace_args() and causes subsequent dynamic arguments to be truncated or fail with -ENOMEM. Should fetch_store_symstrlen() and fetch_store_symstring() use matching symbol formatting logic (or "%pB" if descriptor dereferencing is not desired)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
