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

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

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

While looking at tasks global descriptor tables, I discovered the
following anomaly:

~# echo 't:sched sched_switch prev=prev mm=(task_struct)prev->mm ppgd=(task_struct)prev->mm->pgd' > /sys/kernel/tracing/dynamic_events

The above creates a tprobe on the sched_switch tracepoint and records the
previous task_struct pointer, it's mm_struct and the pgd the mm_struct
points to.

~# trace-cmd start -e tracepoints:sched

            bash-1050    [000] d..3.    90.829478: sched: (__probestub_sched_switch+0x0/0x10) prev=0xffff88d30b623600 mm=0xffff88d30108c600 ppgd=0xffff88d33352b000
          <idle>-0       [002] d..3.    90.836024: sched: (__probestub_sched_switch+0x0/0x10) prev=0xffff88d301870000 mm=0x0 ppgd=0x0
     rcu_preempt-15      [002] d..3.    90.836031: sched: (__probestub_sched_switch+0x0/0x10) prev=0xffff88d3013ad100 mm=0x0 ppgd=0xffff88d33352b000

The first event has is a user task that has both a valid mm_struct and a pgd.
The second event is a kernel task where the mm_struct is NULL and so is the
dereferenced pgd pointer.

The third event is where things looked strange. The mm_struct is NULL but
the dereferenced pgd has a value! That is: NULL->pgd produced something
other than zero.

The reason for this is that the probe_mem_read() and probe_mem_read_user()
do not write to the destination on failure. The process_fetch_insn_bottom()
loop does not check the return value of the dereference (as it is the last
one), and does not update the event. That leaves the value for that failed
field to be whatever was in the ring buffer at that time.

Have both probe_mem_read() and probe_mem_read_user() set the destination to
zero on failure to read the address.

Now all pgd are zero when mm is zero.

Also update the stage 2 of process_fetch_insn_bottom() to not return on
fault but to just set the value to zero and also do not even bother
dereferencing the address if the value is already zero. This allows the
code to continue and fill in the zero into the memory location on the
ring buffer.

Cc: stable@vger.kernel.org
Fixes: 533059281ee59 ("tracing: probeevent: Introduce new argument fetching code")
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
Changse since v1: https://patch.msgid.link/20261111050322.49bbebfa@fedora

- Have stage two handle faults as well, and just ignore dereferencing NULL
  pointers. (sashiko)

 kernel/trace/trace_probe_kernel.h | 13 +++++++++++--
 kernel/trace/trace_probe_tmpl.h   | 15 ++++++++-------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace_probe_kernel.h b/kernel/trace/trace_probe_kernel.h
index 4eb4acfa09ba..00542a162d30 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;
 }
 
 #endif /* __TRACE_PROBE_KERNEL_H_ */
diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index 8db12f758fda..210a75ed5a92 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;
 		case FETCH_OP_CPU_PTR:
 			val = (unsigned long)this_cpu_ptr((void __percpu *)val);
-			ret = 0;
 			break;
 		default:
 			lval = llval;
 			goto out;
 		}
-		if (ret)
-			return ret;
 		llval = lval;
 		code++;
 	} while (1);
-- 
2.53.0

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

[sashiko-bot]

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