From: Mateusz Guzik <mjguzik@gmail.com>
Subject: Re: fstat sysctl crashes kernel
To: Theo de Raadt <deraadt@openbsd.org>
Cc: tech@openbsd.org, millert@openbsd.org, alexander.bluhm@gmx.net
Date: Thu, 8 Aug 2024 19:44:56 +0200

On Thu, Aug 08, 2024 at 03:47:27PM +0000, Theo de Raadt wrote:
> Todd C. Miller <millert@openbsd.org> wrote:
> 
> > On Thu, 08 Aug 2024 13:21:54 +0200, Alexander Bluhm wrote:
> > 
> > > Independent of that, can we commit the chunk below?  When searching
> > > for a specific process, there is not need to traverse the list to
> > > the end.  Also I check for arg >= 0 as this is cosistent with the
> > > arg < -1 check before.  Then you don't have to think about what is
> > > special about process 0.  There is no functional difference as
> > > process 0 has PS_SYSTEM set and is skipped anyway.
> > 
> > Yes, this makes sense. OK millert@
> 
> No matter what we do, we don't get an atomic view.  Punting the problem
> to userland sidesteps the difficult problem in the kernel.

I just realized literally everyone can call into this code and get the
info, the only restriction is not showing kernel pointers to non-root.

I would expect only root to be able to dump the entire list, while non-0
users would only see their own processes. I verified both Linux and
FreeBSD restrict equivalent functionality in this vein.

That aside the BYPID case dumps text/cdir/rdir/trace, while BYUID
duplicates the code except it lacks text (I'm assuming that's not
intentional).

If filtering by pid, why even bother walking the entire list looking for
it when prfind can use pidhash. This would require some refactoring to
prevent more code duplication.

So I would instead suggest:
1. a routine which can export stuff from a picked process
2. a routine which performs the lookup and walk, it would accept a
callback for filtering (pid or uid)

then case KERN_FILE_BYPID could be +/-:
	if (arg > 0) {
		proc = prfind(arg);
		error = nicely_named_export_by_proc(proc);
		break;
	}
	....
	kern_file_iterator(kern_file_bypid_filter, arg);
	break;

the by uid case would roll with kern_file_iterator(kern_file_byuid_filter, arg);

I will also note that the entire thing issues repeat copyout calls.
that's pretty slow due to repeat SMAP-disabelement trips -- it can
happen in one go per process

that's on top of what I said concerning the safety of the entire thing,
see https://marc.info/?l=openbsd-tech&m=172312202128451&w=2