From: Alexandr Nedvedicky Subject: stacktrace_save_utrace() should be more robust To: tech@openbsd.org Date: Wed, 21 May 2025 11:52:26 +0200 Hello, I've hit a kernel crash when I was playing with btrace(8). The traced program is openssl application which is linked with openssl library. The library itself is compiled with functions which are implemented in assembly. Those functions seem to corrupt a stack frame when they are entered. The kernel crashes here at line 317: 311 312 if (frame == NULL) 313 break; 314 if (INKERNEL(f.f_retaddr)) { 315 if (frame <= lastframe) 316 break; 317 f = *frame; 318 continue; 319 } 320 if (!INKERNEL(lastframe) && frame <= lastframe) 321 break; 322 if (copyin(frame, &f, sizeof(f)) != 0) 323 break; at line 317 we may be reading memory which comes from userland via copyin at line 322 (the code in question runs in while loop). the f.f_retaddr might be corrupted/invalid. The proposed fix below makes sure we treat all frames as userland frames as soon as we call copyin() for the first time. I was able to trigger panic on i386, but I think amd64 suffers from the same issue. thanks and 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..17a0d442382 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; @@ -312,7 +313,7 @@ stacktrace_save_utrace(struct stacktrace *st) if (frame == NULL) break; - if (INKERNEL(f.f_retaddr)) { + if (userland == 0 && INKERNEL(f.f_retaddr)) { if (frame <= lastframe) break; f = *frame; @@ -322,6 +323,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..3a851743088 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; @@ -311,7 +312,10 @@ stacktrace_save_utrace(struct stacktrace *st) if (frame == NULL) break; - if (INKERNEL(f.f_retaddr)) { + /* + * Don't trust f.f_retaddr if it came from userland via copyin + */ + if (userland == 0 && INKERNEL(f.f_retaddr)) { if (frame <= lastframe) break; f = *frame; @@ -321,6 +325,7 @@ stacktrace_save_utrace(struct stacktrace *st) break; if (copyin(frame, &f, sizeof(f)) != 0) break; + userland = 1; } curcpu()->ci_inatomic--; }