Re: [PATCH] tracing/probes: Zero out faulted dereferenced fields

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] tracing/probes: uninitialized field on stage 2 fault in
  store_trace_args()
--

commit 721571368356f203544d8ff8abc91509e1c7eea8
Author: Steven Rostedt <rostedt@goodmis.org>

tracing/probes: Zero out faulted dereferenced fields

This commit zeroes the destination buffer in probe_mem_read() and
probe_mem_read_user() when a nofault memory read fails so that failed
dereferences do not leave stale ring buffer contents in trace event fields.

> diff --git a/kernel/trace/trace_probe_kernel.h b/kernel/trace/trace_probe_kernel.h
> index 4eb4acfa09ba1..00542a162d307 100644
> --- a/kernel/trace/trace_probe_kernel.h
> +++ b/kernel/trace/trace_probe_kernel.h
> @@ -102,18 +102,27 @@ static nokprobe_inline int
>  probe_mem_read_user(void *dest, void *src, size_t size)
>  {
>  	const void __user *uaddr =  (__force const void __user *)src;
> +	int ret;
>  
> -	return copy_from_user_nofault(dest, uaddr, size);
> +	ret = copy_from_user_nofault(dest, uaddr, size);
> +	if (ret < 0)
> +		memset(dest, 0, size);
> +	return ret;
>  }
>  
>  static nokprobe_inline int
>  probe_mem_read(void *dest, void *src, size_t size)
>  {
> +	int ret;
> +
>  #ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>  	if ((unsigned long)src < TASK_SIZE)
>  		return probe_mem_read_user(dest, src, size);
>  #endif
> -	return copy_from_kernel_nofault(dest, src, size);
> +	ret = copy_from_kernel_nofault(dest, src, size);
> +	if (ret < 0)
> +		memset(dest, 0, size);
> +	return ret;
>  }

[Severity: Medium]
Does this still leave the trace event field uninitialized in the ring
buffer when an intermediate dereference faults in stage 2 of
process_fetch_insn_bottom()?

In the commit message's "(task_struct)prev->mm->pgd" example, prev is
valid and prev->mm is NULL, so stage 2 (FETCH_OP_DEREF) succeeds and loads
val = 0, and the fault happens on the final dereference in stage 3
(FETCH_OP_ST_MEM), where dest points to the ring buffer field.

However, if a chain has another dereference after the NULL pointer (such as
"(task_struct)prev->mm->exe_file->f_inode" when prev->mm is NULL), the
fault happens in stage 2:

kernel/trace/trace_probe_tmpl.h:process_fetch_insn_bottom() {
    ...
stage2:
	/* 2nd stage: dereference memory if needed */
	do {
		lval = val;
		switch (code->op) {
		case FETCH_OP_DEREF:
			ret = probe_mem_read(&val, (void *)val + code->offset,
					     sizeof(val));
			break;
		case FETCH_OP_UDEREF:
			ret = probe_mem_read_user(&val,
				 (void *)val + code->offset, sizeof(val));
			break;
    ...
		}
		if (ret)
			return ret;
    ...
}

Here probe_mem_read() only zeroes the stack variable val, and
process_fetch_insn_bottom() returns early before reaching stage 3.
Because store_trace_args() ignores a negative return value from
process_fetch_insn(), the destination field in the ring buffer is never
written to and retains stale ring buffer data (or, for a dynamic string
argument, keeps the pre-initialized *dl = make_data_loc(maxlen, ...)
header without calling set_data_loc()).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111050322.49bbebfa@fedora?part=1