From: "Ted Unangst" <tedu@tedunangst.com>
Subject: Re: stacktrace_save_utrace() should be more robust
To: "Alexandr Nedvedicky" <sashan@fastmail.net>
Cc: "Mark Kettenis" <mark.kettenis@xs4all.nl>, tech@openbsd.org
Date: Wed, 21 May 2025 20:15:48 -0400

On 2025-05-21, Alexandr Nedvedicky wrote:
> I'm afraid I don't follow. I'm going to paste the complete
> stacktrace_save_utrace() function here after applying the latest diff:

>     18		while (st->st_count < STACKTRACE_MAX) {
>     19			/*
>     20			 * Don't trust f.f_retaddr if it came from userland via copyin
>     21			 */
>     22			if (userland == 1 && INKERNEL(f.f_retaddr))
>     23				break;

let's assume userland == 1, and we've copied in the frame.
it has a valid f_retaddr.

>     24	
>     25			if (f.f_retaddr != 0 && !INKERNEL(f.f_retaddr))
>     26				st->st_pc[st->st_count++] = f.f_retaddr;
>     27	
>     28			lastframe = frame;
>     29			frame = f.f_frame;

frame is now an unchecked userland pointer.

>     30	
>     31			if (frame == NULL)
>     32				break;
>     33			if (INKERNEL(f.f_retaddr)) {
>     34				if (frame <= lastframe)
>     35					break;
>     36				f = *frame;
>     37				continue;

The deref here is guarded by another inkernel check, but I'm scared this
code will get shuffled around and the bug reintroduced.

Is it possible to use two loops here? One loop to get out of the kernel,
and then the second that does the copyin? I think there is not so much
code in common that duplicating is a problem, and it will be more clear
what we're trying to do in each loop.