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

> Date: Wed, 21 May 2025 16:02:13 +0200
> From: Alexandr Nedvedicky <sashan@fastmail.net>
> 
> 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.

But you just moved the f.f_retaddr check, I'm talking about adding an
f.f_frame check.

> 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--;
>  }
>