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 13:11:40 +0200

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

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.

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