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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] tracing/probes: stage 3 in process_fetch_insn_bottom()
  dereferences NULL val
- [Medium] tracing/probes: FETCH_OP_CPU_PTR in
  process_fetch_insn_bottom() shifts NULL
- [Medium] tracing/probes: FETCH_OP_LP_ARRAY in
  process_fetch_insn_bottom() advances NULL
- [Low] tracing/probes: trace_uprobe probe_mem_read() leaves partial
  bytes on fault
--

commit 0adbcc076a7e0ebaeea272396034e57fe028eb89
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() on read failure, and updates stage 2 of
process_fetch_insn_bottom() to skip NULL pointer dereferences and continue
instead of returning on fault.

> 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: Low]
Does probe_mem_read() in kernel/trace/trace_uprobe.c also need to zero dest
when copy_from_user() fails?

kernel/trace/trace_uprobe.c defines its own probe_mem_read() and includes
kernel/trace/trace_probe_tmpl.h:

kernel/trace/trace_uprobe.c:probe_mem_read() {
	void __user *vaddr = (void __force __user *)src;

	return copy_from_user(dest, vaddr, size) ? -EFAULT : 0;
}

On a partial fault across a page boundary, _inline_copy_from_user() only
zeroes the uncopied tail bytes, leaving the initial copied bytes in dest.
Because stage 2 in process_fetch_insn_bottom() now ignores the return value
of probe_mem_read(), an unaligned uprobe dereference that faults partway
through a pointer leaves a partially written val that subsequent stage 2 or
stage 3 operations continue to dereference or store.

> diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
> index 8db12f758fdae..210a75ed5a92f 100644
> --- a/kernel/trace/trace_probe_tmpl.h
> +++ b/kernel/trace/trace_probe_tmpl.h
> @@ -137,23 +137,24 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
>  		lval = val;
>  		switch (code->op) {
>  		case FETCH_OP_DEREF:
> -			ret = probe_mem_read(&val, (void *)val + code->offset,
> -					     sizeof(val));
> +			/* Do not dereference NULL pointers */
> +			if (val)
> +				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));
> +			/* Do not dereference NULL pointers */
> +			if (val)
> +				probe_mem_read_user(&val, (void *)val + code->offset,
> +						    sizeof(val));
>  			break;

[Severity: Medium]
When val is 0 (or becomes 0 after a stage 2 fault), doesn't stage 3 still
dereference (void *)0 + code->offset?

finalize_fetch_insn() converts the final FETCH_OP_DEREF or FETCH_OP_UDEREF
of a dereference chain (such as ->pgd in prev->mm->pgd) into
FETCH_OP_ST_MEM, FETCH_OP_ST_UMEM, FETCH_OP_ST_STRING, or
FETCH_OP_ST_USTRING in stage 3, which do not check if (val):

kernel/trace/trace_probe_tmpl.h:process_fetch_insn_bottom() {
    ...
	switch (code->op) {
	case FETCH_OP_ST_RAW:
		fetch_store_raw(val, code, dest);
		break;
	case FETCH_OP_ST_MEM:
		probe_mem_read(dest, (void *)val + code->offset, code->size);
		break;
	case FETCH_OP_ST_UMEM:
		probe_mem_read_user(dest, (void *)val + code->offset, code->size);
		break;
	case FETCH_OP_ST_STRING:
		loc = *(u32 *)dest;
		ret = fetch_store_string(val + code->offset, dest, base);
		break;
    ...
}

When val is 0, stage 3 still attempts to read from (void *)0 + code->offset.
On architectures with CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE,
probe_mem_read(), fetch_store_strlen(), and fetch_store_string() see
(unsigned long)src < TASK_SIZE for positive offsets and redirect this NULL
kernel pointer dereference to user space via probe_mem_read_user() or
fetch_store_string_user(), which raises a page fault on every NULL pointer
event and copies unrelated user-space memory into the trace event if that
address is mapped in current.  In addition, when a stage 2 dereference
preceding FETCH_OP_ST_SYMSTR faults, continuing to stage 3 with val == 0
passes 0 + code->offset to fetch_store_symstrlen() and
fetch_store_symstring(), formatting 0x0 instead of failing.

>  		case FETCH_OP_CPU_PTR:
>  			val = (unsigned long)this_cpu_ptr((void __percpu *)val);
> -			ret = 0;
>  			break;

[Severity: Medium]
Should FETCH_OP_CPU_PTR also check if (val) before calling this_cpu_ptr()?

In expressions such as this_cpu_ptr(+0($arg1)) or this_cpu_read(+0($arg1)),
unwind_parse_states() emits FETCH_OP_DEREF followed by FETCH_OP_CPU_PTR.
Now that stage 2 continues with val == 0 when the preceding FETCH_OP_DEREF
reads NULL or faults, FETCH_OP_CPU_PTR computes this_cpu_ptr(NULL), turning
val into the non-zero per-CPU base offset (my_cpu_offset on SMP) so
subsequent instructions record or dereference my_cpu_offset + code->offset
instead of 0.

>  		default:
>  			lval = llval;
>  			goto out;
>  		}
> -		if (ret)
> -			return ret;
>  		llval = lval;
>  		code++;
>  	} while (1);

[Severity: Medium]
Can continuing past stage 2 when val is 0 cause FETCH_OP_LP_ARRAY to advance
from NULL into non-zero addresses on subsequent array elements?

kernel/trace/trace_probe_tmpl.h:process_fetch_insn_bottom() {
    ...
array:
	/* the last stage: Loop on array */
	if (code->op == FETCH_OP_LP_ARRAY) {
		if (ret < 0)
			ret = 0;
		total += ret;
		if (++i < code->param) {
			code = s3;
			if (s3->op != FETCH_OP_ST_STRING &&
			    s3->op != FETCH_OP_ST_USTRING) {
				dest += s3->size;
				val += s3->size;
				goto stage3;
			}
			code--;
			val = lval + sizeof(char *);
			if (dest) {
				dest += sizeof(u32);
				*(u32 *)dest = update_data_loc(loc, ret);
			}
			goto stage2;
		}
    ...
}

When the base pointer of an array argument is NULL or a stage 2 dereference
faults, execution now reaches array: with val == 0 (or lval == 0).  On the
next iteration, scalar arrays increment val by s3->size and jump to stage3,
while string arrays set val = lval + sizeof(char *) and jump to stage2 where
val is now non-zero, bypassing the if (val) check and dereferencing
sizeof(char *) + code->offset.

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