tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kernel crash, struct ptimer and callouts
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);
}
Home |
Main Index |
Thread Index |
Old Index