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 <[email protected]> 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/[email protected]?part=1
