Index | Thread | Search

From:
Hans-Jörg Höxer <hshoexer@genua.de>
Subject:
Re: [EXT] Re: Kernel protection fault in fill_kproc()
To:
<tech@openbsd.org>
Cc:
Gerhard Roth <gerhard_roth@genua.de>, "mvs@openbsd.org" <mvs@openbsd.org>, Carsten Beckmann <carsten_beckmann@genua.de>, "mark.kettenis@xs4all.nl" <mark.kettenis@xs4all.nl>
Date:
Mon, 22 Sep 2025 18:04:50 +0200

Download raw body.

Thread
Hi,

On Thu, Sep 18, 2025 at 06:10:23PM +0200, Martin Pieuchot wrote:
> On 12/09/25(Fri) 13:09, cjeker@diehard.n-r-g.com wrote:
> > [...] 
> > I prefer an explicit if (vm == NULL) return; check in uvmspace_free() to
> > make this more obvious. Since that's just optics you can decide.
> 
> Here's an updated version with the NULL check updated.
> 
> Gerhard would you like to ok the diff?  Or at least confirm it fixes the
> bug you originally reported?  Thanks!

we have tested your diff and we can confirm, that it fixes the bug
Gerhard has reported.  Thanks!

> Index: kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> diff -u -p -r1.482 kern_sysctl.c
> --- kern/kern_sysctl.c	6 Aug 2025 14:00:33 -0000	1.482
> +++ kern/kern_sysctl.c	18 Sep 2025 16:03:41 -0000
> @@ -2051,11 +2051,17 @@ fill_kproc(struct process *pr, struct ki
>  {
>  	struct session *s = pr->ps_session;
>  	struct tty *tp;
> -	struct vmspace *vm = pr->ps_vmspace;
> +	struct vmspace *vm = NULL;
>  	struct timespec booted, st, ut, utc;
>  	struct tusage tu;
>  	int isthread;
>  
> +	/* exiting/zombie process might no longer have VM space. */
> +	if ((pr->ps_flags & PS_EXITING) == 0) {
> +		vm = pr->ps_vmspace;
> +		uvmspace_addref(vm);
> +	}
> +
>  	isthread = p != NULL;
>  	if (!isthread) {
>  		p = pr->ps_mainproc;		/* XXX */
> @@ -2082,7 +2088,7 @@ fill_kproc(struct process *pr, struct ki
>  	}
>  
>  	/* fixups that can only be done in the kernel */
> -	if ((pr->ps_flags & PS_ZOMBIE) == 0) {
> +	if ((pr->ps_flags & PS_EXITING) == 0) {
>  		if ((pr->ps_flags & PS_EMBRYO) == 0 && vm != NULL)
>  			ki->p_vm_rssize = vm_resident_count(vm);
>  		calctsru(&tu, &ut, &st, NULL);
> @@ -2103,13 +2109,15 @@ fill_kproc(struct process *pr, struct ki
>  #endif
>  	}
>  
> +	uvmspace_free(vm);
> +
>  	/* get %cpu and schedule state: just one thread or sum of all? */
>  	if (isthread) {
>  		ki->p_pctcpu = p->p_pctcpu;
>  		ki->p_stat   = p->p_stat;
>  	} else {
>  		ki->p_pctcpu = 0;
> -		ki->p_stat = (pr->ps_flags & PS_ZOMBIE) ? SDEAD : SIDL;
> +		ki->p_stat = (pr->ps_flags & PS_EXITING) ? SDEAD : SIDL;
>  		TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
>  			ki->p_pctcpu += p->p_pctcpu;
>  			/* find best state: ONPROC > RUN > STOP > SLEEP > .. */
> Index: kern/tty.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/tty.c,v
> diff -u -p -r1.180 tty.c
> --- kern/tty.c	12 Jun 2025 20:37:58 -0000	1.180
> +++ kern/tty.c	18 Sep 2025 16:03:41 -0000
> @@ -2198,8 +2198,8 @@ empty:		ttyprintf(tp, "empty foreground 
>  			if (run2 || pctcpu2 > pctcpu)
>  				goto update_pickpr;
>  
> -			/* if p has less cpu or is zombie, then it's worse */
> -			if (pctcpu2 < pctcpu || (pr->ps_flags & PS_ZOMBIE))
> +			/* if p has less cpu or is exiting, then it's worse */
> +			if (pctcpu2 < pctcpu || (pr->ps_flags & PS_EXITING))
>  				continue;
>  update_pickpr:
>  			pickpr = pr;
> @@ -2209,7 +2209,7 @@ update_pickpr:
>  
>  		/* Calculate percentage cpu, resident set size. */
>  		calc_pctcpu = (pctcpu * 10000 + FSCALE / 2) >> FSHIFT;
> -		if ((pickpr->ps_flags & (PS_EMBRYO | PS_ZOMBIE)) == 0 &&
> +		if ((pickpr->ps_flags & (PS_EMBRYO | PS_EXITING)) == 0 &&
>  		    pickpr->ps_vmspace != NULL)
>  			rss = vm_resident_count(pickpr->ps_vmspace);
>  
> Index: uvm/uvm_map.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> diff -u -p -r1.346 uvm_map.c
> --- uvm/uvm_map.c	11 Sep 2025 17:04:35 -0000	1.346
> +++ uvm/uvm_map.c	18 Sep 2025 16:04:20 -0000
> @@ -3426,6 +3426,9 @@ uvmspace_purge(struct vmspace *vm)
>  void
>  uvmspace_free(struct vmspace *vm)
>  {
> +	if (vm == NULL)
> +		return;
> +
>  	if (atomic_dec_int_nv(&vm->vm_refcnt) == 0) {
>  		/*
>  		 * Sanity check.  Kernel threads never end up here and
> 
> 

-- 
Dr. Hans-Jörg Höxer                   Hans-Joerg_Hoexer@genua.de
Senior Expert Kryptographie
eXtreme Kernel and Crypto Development

genua GmbH
Domagkstrasse 7, 85551 Kirchheim bei München
tel +49 89 991950-0, fax -999, www.genua.eu

Geschäftsführer: Matthias Ochs, Marc Tesch
Amtsgericht München HRB 98238
genua ist ein Unternehmen der Bundesdruckerei-Gruppe.