Re: [PATCH v13 05/12] tracing/wprobe: Add wprobe (watchpoint probe) trace event support

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.wangjinchao600@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