From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: Re: stacktrace_save_utrace() should be more robust
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: tech@openbsd.org
Date: Wed, 21 May 2025 16:02:13 +0200

Hello,

On Wed, May 21, 2025 at 01:11:40PM +0200, Mark Kettenis wrote:

</snip>
> 
> That seems like a reasonable assumption.  But we should probably avoid
> the copyin() altogether (and just terminate the trace) if f.f_frame is
> a kernel address.  So something like:
> 
>                 if (userland == 1 && INKERNEL(f.f_frame))
> 		        break;
> 
> somewhere near the top of the while loop.
> 

    yes, it makes perfect sense. thanks for the suggestion.
    updated diff follows. it still works on i386.

regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/arch/amd64/amd64/db_trace.c b/sys/arch/amd64/amd64/db_trace.c
index 1e7fcf2208a..b3891efc54d 100644
--- a/sys/arch/amd64/amd64/db_trace.c
+++ b/sys/arch/amd64/amd64/db_trace.c
@@ -292,6 +292,7 @@ stacktrace_save_utrace(struct stacktrace *st)
 {
 	struct callframe f, *frame, *lastframe;
 	struct pcb *pcb = curpcb;
+	int userland = 0;
 
 	st->st_count = 0;
 
@@ -304,6 +305,9 @@ stacktrace_save_utrace(struct stacktrace *st)
 
 	curcpu()->ci_inatomic++;
 	while (st->st_count < STACKTRACE_MAX) {
+		if (userland == 1 && INKERNEL(f.f_retaddr))
+			break;
+
 		if (f.f_retaddr != 0 && !INKERNEL(f.f_retaddr))
 			st->st_pc[st->st_count++] = f.f_retaddr;
 
@@ -322,6 +326,7 @@ stacktrace_save_utrace(struct stacktrace *st)
 			break;
 		if (copyin(frame, &f, sizeof(f)) != 0)
 			break;
+		userland = 1;
 	}
 	curcpu()->ci_inatomic--;
 }
diff --git a/sys/arch/i386/i386/db_trace.c b/sys/arch/i386/i386/db_trace.c
index 2e1c9faefca..221909077f4 100644
--- a/sys/arch/i386/i386/db_trace.c
+++ b/sys/arch/i386/i386/db_trace.c
@@ -291,6 +291,7 @@ stacktrace_save_utrace(struct stacktrace *st)
 {
 	struct callframe f, *frame, *lastframe;
 	struct pcb *pcb = curpcb;
+	int userland = 0;
 
 	st->st_count = 0;
 
@@ -303,6 +304,9 @@ stacktrace_save_utrace(struct stacktrace *st)
 
 	curcpu()->ci_inatomic++;
 	while (st->st_count < STACKTRACE_MAX) {
+		if (userland == 1 && INKERNEL(f.f_retaddr))
+			break;
+
 		if (f.f_retaddr != 0 && !INKERNEL(f.f_retaddr))
 			st->st_pc[st->st_count++] = f.f_retaddr;
 
@@ -311,6 +315,9 @@ stacktrace_save_utrace(struct stacktrace *st)
 
 		if (frame == NULL)
 			break;
+		/*
+		 * Don't trust f.f_retaddr if it came from userland via copyin
+		 */
 		if (INKERNEL(f.f_retaddr)) {
 			if (frame <= lastframe)
 				break;
@@ -321,6 +328,7 @@ stacktrace_save_utrace(struct stacktrace *st)
 			break;
 		if (copyin(frame, &f, sizeof(f)) != 0)
 			break;
+		userland = 1;
 	}
 	curcpu()->ci_inatomic--;
 }