Bug 11399

Summary: (patch queued)possible buffer underflow in ib700wdt.c
Product: Drivers Reporter: Zvonimir Rakamaric (zrakamar)
Component: WatchdogAssignee: Alan (alan)
Status: CLOSED CODE_FIX    
Severity: normal CC: akpm, wim, zrakamar
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.26 Subsystem:
Regression: --- Bisected commit-id:
Attachments: Patch to fix buffer_underflow

Description Zvonimir Rakamaric 2008-08-21 17:38:09 UTC
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;
}
Comment 1 Andrew Morton 2008-08-26 16:33:09 UTC
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.
Comment 2 Alan 2008-09-22 09:40:40 UTC
Agreed with the analysis. Patch queued
Comment 3 Wim Van Sebroeck 2008-09-23 01:46:46 UTC
Need to check if the bug can be simply fixed by changing
"if (wd_times[i] > t)" to "if (wd_times[i] >= t)"
Comment 4 Wim Van Sebroeck 2008-10-15 01:49:05 UTC
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.
Comment 5 Wim Van Sebroeck 2008-10-15 04:29:05 UTC
Created attachment 18323 [details]
Patch to fix buffer_underflow
Comment 6 Wim Van Sebroeck 2008-12-18 23:49:09 UTC
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.