Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The trace_wprobe_show function uses %pS to format the symbol address, breaking dynamic event restoration because kstrtoint() in parse_address_spec cannot parse the appended symbol size. - [Medium] Address validation in parse_address_spec incorrectly exempts address 0 from the kernel space TASK_SIZE check, allowing user-space tracing at address 0. --
--- Patch [5]: [PATCH v13 05/12] tracing/wprobe: Add wprobe (watchpoint probe) trace event support --- commit 8eff80da8ae4225daa9b5646b430ded1a73ae7ae Author: Masami Hiramatsu (Google) <[email protected]> tracing/wprobe: Add wprobe (watchpoint probe) trace event support Add hardware-breakpoint-based dynamic trace event support (wprobe). Wprobe creates a dynamic event on data read/write accesses using hardware breakpoints and logs the access context and fetchargs. Link: https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao...@gmail.com/ > diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c > new file mode 100644 > index 0000000000000..a8543766a245b > --- /dev/null > +++ b/kernel/trace/trace_wprobe.c [ ... ] > +static int parse_address_spec(const char *spec, unsigned long *addr, int > *type, > + int *len, char **symbol) > +{ [ ... ] > + if (kstrtoul(at + 1, 0, &_addr) != 0) { > + char *off_str = strpbrk(at + 1, "+-"); > + int offset = 0; > + > + if (off_str) { > + if (kstrtoint(off_str, 0, &offset) != 0) { > + trace_probe_log_err(off_str - _spec, > BAD_PROBE_ADDR); > + return -EINVAL; > + } > + *off_str = '\0'; > + } > + _addr = kallsyms_lookup_name(at + 1); > + if (!_addr) { > + trace_probe_log_err(at + 1 - _spec, BAD_ACCESS_ADDR); > + return -ENOENT; > + } > + _addr += offset; > + *symbol = kstrdup(at + 1, GFP_KERNEL); > + if (!*symbol) > + return -ENOMEM; > + } > + > + if (_addr != 0 && _addr < TASK_SIZE) { [Severity: Medium] Does this allow user-space tracing at address 0? The condition explicitly exempts _addr == 0. If a user specifies an address of 0, it appears this would bypass the TASK_SIZE check and successfully allow setting a hardware breakpoint in user space. > + trace_probe_log_err(at + 1 - _spec, BAD_ACCESS_ADDR); > + return -EINVAL; > + } > + > + *addr = _addr; > + *type = _type; > + *len = _len; > + return 0; > +} [ ... ] > +static int trace_wprobe_show(struct seq_file *m, struct dyn_event *ev) > +{ [ ... ] > + if (tw->len == HW_BREAKPOINT_LEN_1) > + len = 1; > + else if (tw->len == HW_BREAKPOINT_LEN_2) > + len = 2; > + else if (tw->len == HW_BREAKPOINT_LEN_4) > + len = 4; > + else > + len = 8; > + > + if (tw->symbol) > + seq_printf(m, " %s@%pS:%d", type_str, (void *)tw->addr, len); [Severity: High] Will formatting the address with %pS break dynamic event restoration? Using %pS appends the symbol size to the output string, which results in formats like symbol+0x0/0x100. When this string is saved and written back to restore the event, parse_address_spec() extracts the offset part (+0x0/0x100) and passes it to kstrtoint(). Since kstrtoint() requires a strict integer format, won't it fail on the /size suffix and return -EINVAL, preventing restoration? > + else > + seq_printf(m, " %s@0x%lx:%d", type_str, tw->addr, len); > + > + for (i = 0; i < tw->tp.nr_args; i++) > + seq_printf(m, " %s=%s", tw->tp.args[i].name, > tw->tp.args[i].comm); > + seq_putc(m, '\n'); > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/178739053919.1520941.17662338993878200834.stgit@devnote2?part=5
