panic caused by EVFILT_SIGNAL detaching in rfork()ed thread

[Uwe Doering]

Igor Sysoev wrote:
> Here is more correct patch to fix the panic in 4.x reported in
> http://freebsd.rambler.ru/bsdmail/freebsd-hackers_2004/msg02732.html
> 
> -------------------------
> --- src/sys/kern/kern_event.c   Sun Oct 10 12:17:55 2004
> +++ src/sys/kern/kern_event.c   Sun Oct 10 12:19:29 2004
> @@ -794,7 +794,8 @@
>             while (kn != NULL) {
>                 kn0 = SLIST_NEXT(kn, kn_link);
>                 if (kq == kn->kn_kq) {
> -                   kn->kn_fop->f_detach(kn);
> +                   if (!(kn->kn_status & KN_DETACHED))
> +                       kn->kn_fop->f_detach(kn);
>         /* XXX non-fd release of kn->kn_ptr */
>                     knote_free(kn);
>                     *knp = kn0;
> -------------------------

Your patch appears to be an excerpt from the fix to RELENG_5.  May I 
suggest a different approach for RELENG_4?  My reasoning is that the 
implementation of kevents differs between RELENG_4 and RELENG_5.

In RELENG_5 the flag KN_DETACHED is used in a more general way.  It gets 
set by knlist_add() and cleared by knlist_remove(), in sync with list 
insertion and removal.  As far as I can tell these routines have 
originally been introduced in order to centralize the locking for kevent 
list manipulations, and they don't exist in RELENG_4.

Now, the proper way to MFC the RELENG_5 fix to RELENG_4 more or less 
unchanged would be to MFC the whole knlist_add()/knlist_remove() 
business as well (w/o the locking stuff), which, however, would be 
overkill for RELENG_4's single threaded kernel.

In RELENG_4's implementation of kevents, the only case in which 
KN_DETACHED gets set is when a process exits and posts a NOTE_EXIT 
event.  That is, the meaning of KN_DETACHED is much narrower than in 
RELENG_5.  For this reason I believe the most appropriate fix would be 
to check for KN_DETACHED in filt_sigdetach() in the same way it is 
already done in filt_procdetach().  In fact, if you compare the two 
routines it becomes pretty obvious that they should have been identical 
in the first place, and that the absence of said check from 
filt_sigdetach() is most likely just an oversight.

Therefore, I suggest to adopt the attached patch and leave the rest of 
RELENG_4's kevent code alone.  I checked the kernel sources and found 
that filt_procdetach() and filt_sigdetach() are in fact the only 
f_detach() routines that deal with a process' p_klist field, and 
therefore need this kind of safeguard.

Also, it would probably be a good idea to fix RELENG_4 swiftly (and 
possibly release a security advisory) because this flaw is certainly a 
great DoS opportunity for maliciously minded shell users ...

    Uwe
-- 
Uwe Doering         |  EscapeBox - Managed On-Demand UNIX Servers
gemini at geminix.org  |  http://www.escapebox.net
-------------- next part --------------
--- src/sys/kern/kern_sig.c.orig	Thu Feb  5 23:26:48 2004
+++ src/sys/kern/kern_sig.c	Sat Oct 23 11:15:30 2004
@@ -1739,6 +1739,10 @@
 {
 	struct proc *p = kn->kn_ptr.p_proc;
 
+	if (kn->kn_status & KN_DETACHED)
+		return;
+
+	/* XXX locking?  this might modify another process. */
 	SLIST_REMOVE(&p->p_klist, kn, knote, kn_selnext);
 }