Re: In-kernel process exit hooks?

[Mateusz Guzik <mjguzik%gmail.com@localhost>]

On Sat, Jan 09, 2016 at 02:25:02PM +0800, Paul Goyette wrote:
> On Sat, 9 Jan 2016, Mateusz Guzik wrote:
> 
> >While I don't know all the details, it is clear that the purpose would
> >be much better served by ktrace and I would argue efforts should be
> >spent there.
> 
> filemon's purpose is somewhat different than that of ktrace.  There
> is a limited set of events that filemon captures, and it only
> captures successful operations.  Its output is quite simple and easy
> to parse by a human of shell script.
> 

ktrace already have limited support for specifying what events are
wanted. One can add a special filemon-like mode.

The output of kdump is definitely less friendly for scripts, but it is
way easier to write a script formatting it than it is to fix filemon.

> >>From usability perspective what seems to be needed is few hacks to
> >ktrace to only log the stuff make is interested in and parsing code for
> >bmake to translate to what looks like current filemon format for
> >aformentioned copying.
> 
> Perhaps.  But filemon is also a run-time loadable module (and it is
> preferred that it NOT be built-in to any kernels).  If I remember
> correctly, ktrace has hooks all over the system and does not readily
> lend itself to modularization.
> 

I doubt that compiling in ktrace on a which can handle filemon is an
issue, and even then it should outweight hops around filemon.

For correct of a minimalistic safe filemon module the kernel would
already have to grow hooks in few places (see below).


> >Filemon monitors stuff at syscall level, stores the fd and remembers
> >pids.
> 
> The fd is no longer being stored...  :)  But yes, it does still
> remember pids, so it can determine if an event belongs to a monitored
> process tree.  Again IIRC, ktrace is somewhat more intrusive, in that
> it manipulates various process flags which can affect behavior of the
> target (monitored) process.

It has a dedicated field in struct proc which is the only sensible way
of implementing this I can think of.

> >>               error = kauth_authorize_process(curproc->p_cred,
> >>                   KAUTH_PROCESS_CANSEE, tp,
> >>                   KAUTH_ARG(KAUTH_REQ_PROCESS_CANSEE_ENTRY), NULL, NULL);
> >
> >Now that the check is pased the process could have executed something
> >setuid.
> 
> Which process could have executed something setuid?  How would that
> matter?  (I will confess I haven't checked, but I would be surprised
> if "executing something setuid" would change the result of kauth()
> check. But I could be wrong here.)
> 

You start monitoring given process and proceed to execve a setuid file,
which gives you information what it is doing, even though the
kauth_authorize_process check would fail. This is basically what you
yourself stated below about pid reuse.

So this either needs to prevent processes from changing credentials when
traced or stop tracing when such a change occurs. The former seems like
a rather bad idea and latter is already done by ktrace.

> >I'm somewhat surprised this code is here. Should not
> >kauth_authorize_process assert that tp has stable credentials in some
> >way, in effect just crashing filemon consumers?
> >
> >>               if (!error) {
> >>                       filemon->fm_pid = tp->p_pid;
> >>               }
> 
> Why does the target process need stable credentials?  It's not doing
> anything for filemon.
> 

See below.

> >Pid is stored, but tp is not held. What if the 'traced' process exits?
> >grep only reveals the following bit in filemon_wrapper_sys_exit:
> >
> >>               if (filemon->fm_pid == curproc->p_pid)
> >>                       filemon_printf(filemon, "# Bye bye\n");
> >
> >So filemon will continue tracing anything which happened to reuse the
> >pid.
> 
> Yes, this is an outstanding issue.  More particularly, the new user
> of the pid might not be accessible by the monitoring process (ie, it
> would fail the earlier kauth() check).  But holding the pointer to
> the proc isn't really the answer either - although less likely it is
> entirely possible that the address could have been reused.
> 

What you need is the ability to clear tracing bits on process exit.
ktrace definitely has relevant bits in place already.

> (It's too bad we don't have "generation numbers" on the pid, which
> would easily detect reuse.)

That would require struct proc to be type stable, which seems wasteful.

> >>                       lp = p;
> >>                       p = p->p_pptr;
> >>                       rw_exit(&lp->p_reflock);
> >
> >I guess the lock is needed to read the pointer to the parent.
> 
> Correct.
> 
> >Nothing was done to keep the parent around, which means it could have
> >exited and be freed by now. Which in turn makes the second loop
> >iteration a use-after-free.
> 
> Hmmm, this code used to work.  Looks like the locking got messed up
> somewhere.  It should do (in pseudo code - I'm too lazy to write it
> all out!)
> 
> 	lock(p)
> 	while (p != NULL) {
> 		TAILQ_FOREACH(filemon) {
> 			if (p->p_pid == filemon->fm_pid) {
> 				release(p);
> 				return filemon;
> 			}
> 		}
> 		last_p = p;
> 		p = p->pptr;
> 		if (p != NULL)
> 			lock(p);
> 		release(last_p);
> 	}
> 
> We should lock the new (parent) process before releasing the initial
> lock.
> 
> I will fix this.
> 

Just locking the parent like that looks like a deadlock potential.

I just checked both ptrace and exit code. The parent<->child
relationship is protected with proc_lock and p_lock. p_reflock happens
to be held in exit1 and also cover the area, but is not held in ptrace.

Unclear what's the protocol here without proc_lock held. If the sample
from ptrace is to be trusted, it's locking the lower address first (and
thus trylocking the lower address if we already got the higher one).

The sample from sys_ptrace:

>		struct proc *parent = t->p_pptr;
>
>		if (parent->p_lock < t->p_lock) {
>			if (!mutex_tryenter(parent->p_lock)) {
>				mutex_exit(t->p_lock);
>				mutex_enter(parent->p_lock);

As a side note this looks like a bug. t->p_lock is not relocked, so the
code ends up without the lock held. There is a similar sample in fork1.

>			}
>		} else if (parent->p_lock > t->p_lock) {
>			mutex_enter(parent->p_lock);
>		}
>		parent->p_slflag |= PSL_CHTRACED;
>		proc_reparent(t, p);
>		if (parent->p_lock != t->p_lock)
>			mutex_exit(parent->p_lock);
>

That said, even if following p_pptr was the right thing to do, I don't
think this would get away without hacks or holding proc_lock.

While the first iteration uses curproc and could reliably lock the
parent despite possible relocking, the need to keep lower < higher order
may meen you need to temporarily unlock the process, but for that to not
be a problem you need to hold the process and/or restart the loop
possibly with proc_mutex held. Looks very hairy.

With a dedicated pointer stored in struct proc (like with ktrace) there
is no need to do any lookups in the first place, so probles like this
don't arose.

> >
> >The first iteration is safe if the argument is curproc (which it is), as
> >it clearly cannot disappear.
> >
> >Turns out this traversal is even more wrong since p_pptr does not have
> >to be the process you are looking for - ptrace is reparenting tracees.
> 
> If the process has been reparented, then its activity will no longer
> be monitored and reported by filemon.
> 

Sorry, I forgot to state the corner case. I can't test it, but if NetBSD
allows you to trace your parent, the loop above becomes infinite.

Also you can get surprise events from ptrace tracees.

So again, I highly doubt filemon in its current form can be made safe to
use while preserving any advantage over ktrace.

The thing is there is filemon and bmake in FreeBSD as well. While I
don't have the time to modify ktrace for this purpose, I would definitely
help both projects if bmake started moving away from filemon.

However, this is just my $0,03 as I don't contribute to this project.

-- 
Mateusz Guzik <mjguzik gmail.com>