From: "Theo de Raadt" <deraadt@openbsd.org>
Subject: Re: fstat sysctl crashes kernel
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: tech@openbsd.org, millert@openbsd.org, alexander.bluhm@gmx.net
Date: Thu, 08 Aug 2024 11:56:09 -0600

I don't think you are helping with these vague set of proposals.

Mateusz Guzik <mjguzik@gmail.com> wrote:

> 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