Problem Description: Steps to a possible buffer_underflow bug in ib700wdt.c, function ibwdt_ioctl: 1. new margin is loaded using get_user(new_margin, p) in case WDIOC_SETTIMEOUT in the function ibwdt_ioctl. Important: assume that new_margin == 30 2. ibwdt_set_heartbeat(new_margin) is called 3. the check "if ((t < 0) || (t > 30))" in ibwdt_set_heartbeat is not going to fail because t == 30 (t is new_margin) 4. in the loop, the check wd_times[i] > t is never going to be true because none of the wd_times are greater than the value of t (i.e. 30) 5. we are exiting the loop with i == -1 and therefore setting wd_margin to -1 in the line wd_margin = i; 6. we are returning from ibwdt_set_heartbeat and wd_margin is -1 7. we fall through to case WDIOC_GETTIMEOUT 8. we access the wd_times array with wd_margin == -1 on line "return put_user(wd_times[wd_margin], p);" I found this bug using my own static checker for low-level systems software whose codename is Smack, which I am developing as a part of my PhD. For the sake of my research (and also I am currently in the process of writing a paper about Smack), I would greatly appreciate if you could confirm/dispute the bug as soon as possible. Thanks a lot! -- Zvonimir Furthermore, here are the pieces of code which are causing this buffer-underflow bug in ib700wdt.c: static int wd_times[] = { 30, /* 0x0 */ 28, /* 0x1 */ 26, /* 0x2 */ 24, /* 0x3 */ 22, /* 0x4 */ 20, /* 0x5 */ 18, /* 0x6 */ 16, /* 0x7 */ 14, /* 0x8 */ 12, /* 0x9 */ 10, /* 0xA */ 8, /* 0xB */ 6, /* 0xC */ 4, /* 0xD */ 2, /* 0xE */ 0, /* 0xF */ }; function ibwdt_ioctl: ..... case WDIOC_SETTIMEOUT: if (get_user(new_margin, p)) return -EFAULT; if (ibwdt_set_heartbeat(new_margin)) return -EINVAL; ibwdt_ping(); /* Fall */ case WDIOC_GETTIMEOUT: return put_user(wd_times[wd_margin], p); ..... function ibwdt_set_heartbeat: static int ibwdt_set_heartbeat(int t) { int i; if ((t < 0) || (t > 30)) return -EINVAL; for (i = 0x0F; i > -1; i--) if (wd_times[i] > t) break; wd_margin = i; return 0; }
please email a patch ;) There's really no need to report a bug if you feel you can fix it. Just do both at the same time.
Agreed with the analysis. Patch queued
Need to check if the bug can be simply fixed by changing "if (wd_times[i] > t)" to "if (wd_times[i] >= t)"
This is the result for "if (wd_times[i] >= t)": Watchdog timer control table: Level Value Time/sec | Level Value Time/sec 1 F 0 | 9 7 16 2 E 2 | 10 6 18 3 D 4 | 11 5 20 4 C 6 | 12 4 22 5 B 8 | 13 3 24 6 A 10 | 14 2 26 7 9 12 | 15 1 28 8 8 14 | 16 0 30 Time/sec = 30 -> Value = 0x00 Time/sec = 15 -> Value = 0x07 Time/sec = 29 -> Value = 0x00 Time/sec = 14 -> Value = 0x08 Time/sec = 28 -> Value = 0x01 Time/sec = 13 -> Value = 0x08 Time/sec = 27 -> Value = 0x01 Time/sec = 12 -> Value = 0x09 Time/sec = 26 -> Value = 0x02 Time/sec = 11 -> Value = 0x09 Time/sec = 25 -> Value = 0x02 Time/sec = 10 -> Value = 0x0a Time/sec = 24 -> Value = 0x03 Time/sec = 09 -> Value = 0x0a Time/sec = 23 -> Value = 0x03 Time/sec = 08 -> Value = 0x0b Time/sec = 22 -> Value = 0x04 Time/sec = 07 -> Value = 0x0b Time/sec = 21 -> Value = 0x04 Time/sec = 06 -> Value = 0x0c Time/sec = 20 -> Value = 0x05 Time/sec = 05 -> Value = 0x0c Time/sec = 19 -> Value = 0x05 Time/sec = 04 -> Value = 0x0d Time/sec = 18 -> Value = 0x06 Time/sec = 03 -> Value = 0x0d Time/sec = 17 -> Value = 0x06 Time/sec = 02 -> Value = 0x0e Time/sec = 16 -> Value = 0x07 Time/sec = 01 -> Value = 0x0e Time/sec = 15 -> Value = 0x07 Time/sec = 00 -> Value = 0x0f => So fix will go in, in a few minutes.
Created attachment 18323 [details] Patch to fix buffer_underflow
This bug was fixed with: commit 7c2500f17d65092d93345f3996cf82ebca17e9ff Author: Wim Van Sebroeck <wim@iguana.be> Date: Wed Oct 15 08:53:06 2008 +0000 and is in mainline kernel (since v2.6.28-rc1). Can someone close this entry? I can't :-(. Thanks in advance, Wim.