From: Vitaliy Makkoveev Subject: Re: fstat sysctl crashes kernel To: Mateusz Guzik Cc: Theo de Raadt , tech@openbsd.org, millert@openbsd.org, alexander.bluhm@gmx.net Date: Thu, 8 Aug 2024 21:33:21 +0300 What problem are you trying to solve? > On 8 Aug 2024, at 20:44, Mateusz Guzik wrote: > > On Thu, Aug 08, 2024 at 03:47:27PM +0000, Theo de Raadt wrote: >> Todd C. Miller 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 >