Re: kernel crash, struct ptimer and callouts

[christos%astron.com@localhost (Christos Zoulas)]

In article <20080707122033.GM22423%shisha.spb.ru@localhost>,
Alexander Shishkin  <alexander.shishkin%teleca.com@localhost> wrote:
>Hi,
>
>Been wondering recently as to why struct ptimer has a union of callout_t
>and other data fields, namely pt_active and pt_list, given that at least
>timer_create1() dosetitimer() explicitly initialize both pt_ch and
>pt_active. This will lead to a kernel crash whenever a CLOCK_REALTIME
>timer (which has been created and set) is deleted with sys_timer_delete(),
>because timer_settime() will set callout's c_func to realtimerexpire()
>(which will effectively clobber pt_active) and sys_timer_delete() will
>decide to go through pt_list (which is in reality a chain of callouts),
>which in turn will set the system on fire. This pattern [1] is tested and
>valid for i386, other architectures might fail differently depending on
>the alignment and type sizes.
>
>IOW, current netbsd kernel is vulnerable to at local dos attack, which
>sort of, bad. Proposed patch [2] is also appended.
>
>[1]: http://koowaldah.org/people/ash/netbsd/pttest.c
>[2]: http://koowaldah.org/people/ash/netbsd/ptimer-fix.diff

How about this untested patch?

christos

Index: kern_time.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_time.c,v
retrieving revision 1.148
diff -u -u -r1.148 kern_time.c
--- kern_time.c 29 May 2008 15:27:51 -0000      1.148
+++ kern_time.c 7 Jul 2008 16:11:05 -0000
@@ -623,13 +623,16 @@
                mutex_spin_exit(&timer_lock);
                return (EINVAL);
        }
-       if (pt->pt_active) {
-               ptn = LIST_NEXT(pt, pt_list);
-               LIST_REMOVE(pt, pt_list);
-               for ( ; ptn; ptn = LIST_NEXT(ptn, pt_list))
-                       timeradd(&pt->pt_time.it_value, &ptn->pt_time.it_value,
-                           &ptn->pt_time.it_value);
-               pt->pt_active = 0;
+       if (pt->pt_type != CLOCK_REALTIME) {
+               if (pt->pt_active) {
+                       ptn = LIST_NEXT(pt, pt_list);
+                       LIST_REMOVE(pt, pt_list);
+                       for ( ; ptn; ptn = LIST_NEXT(ptn, pt_list))
+                               timeradd(&pt->pt_time.it_value,
+                                   &ptn->pt_time.it_value,
+                                   &ptn->pt_time.it_value);
+                       pt->pt_active = 0;
+               }
        }
        itimerfree(pts, timerid);
 
@@ -1088,9 +1091,13 @@
                pt->pt_proc = p;
                pt->pt_type = which;
                pt->pt_entry = which;
-               pt->pt_active = 0;
                pt->pt_queued = false;
-               callout_init(&pt->pt_ch, CALLOUT_MPSAFE);
+               if (pt->pt_type == CLOCK_REALTIME)
+                       callout_init(&pt->pt_ch, CALLOUT_MPSAFE);
+               else {
+                       pt->pt_active = 0;
+                       LIST_INIT(&pts->pt_list);
+               }
                switch (which) {
                case ITIMER_REAL:
                        pt->pt_ev.sigev_signo = SIGALRM;
@@ -1171,10 +1178,13 @@
                timerclear(&tv);
                for (ptn = LIST_FIRST(&pts->pts_virtual);
                     ptn && ptn != pts->pts_timers[ITIMER_VIRTUAL];
-                    ptn = LIST_NEXT(ptn, pt_list))
+                    ptn = LIST_NEXT(ptn, pt_list)) {
+                       KASSERT(ptn->pt_type != CLOCK_REALTIME);
                        timeradd(&tv, &ptn->pt_time.it_value, &tv);
+               }
                LIST_FIRST(&pts->pts_virtual) = NULL;
                if (ptn) {
+                       KASSERT(ptn->pt_type != CLOCK_REALTIME);
                        timeradd(&tv, &ptn->pt_time.it_value,
                            &ptn->pt_time.it_value);
                        LIST_INSERT_HEAD(&pts->pts_virtual, ptn, pt_list);
@@ -1182,10 +1192,13 @@
                timerclear(&tv);
                for (ptn = LIST_FIRST(&pts->pts_prof);
                     ptn && ptn != pts->pts_timers[ITIMER_PROF];
-                    ptn = LIST_NEXT(ptn, pt_list))
+                    ptn = LIST_NEXT(ptn, pt_list)) {
+                       KASSERT(ptn->pt_type != CLOCK_REALTIME);
                        timeradd(&tv, &ptn->pt_time.it_value, &tv);
+               }
                LIST_FIRST(&pts->pts_prof) = NULL;
                if (ptn) {
+                       KASSERT(ptn->pt_type != CLOCK_REALTIME);
                        timeradd(&tv, &ptn->pt_time.it_value,
                            &ptn->pt_time.it_value);
                        LIST_INSERT_HEAD(&pts->pts_prof, ptn, pt_list);
@@ -1221,7 +1234,8 @@
        else if (pt->pt_queued)
                TAILQ_REMOVE(&timer_queue, pt, pt_chain);
        mutex_spin_exit(&timer_lock);
-       callout_destroy(&pt->pt_ch);
+       if (pt->pt_type == CLOCK_REALTIME)
+               callout_destroy(&pt->pt_ch);
        pool_put(&ptimer_pool, pt);
 }