Skip to contentSkip to conversation

fix read past end of array in md/linear.c

PATCH In review Andy Isaacson 3 participants 5 messages Mar 8, 2007 – Mar 11, 2007 MD RAID

lore
Share & tools
lore

Andy Isaacson

author
#0 lore raw

PATCH 1/1 fix read past end of array in md/linear.c

When iterating through an array, one must be careful to test one's index variable rather than another similarly-named variable.

The loop will read off the end of conf->disks[] in the following (pathological) case:

% dd bs=1 seek=840716287 if=/dev/zero of=d1 count=1
% for i in 2 3 4; do dd if=/dev/zero of=d$i bs=1k count=$(($i+150)); done % ./vmlinux ubd0=root ubd1=d1 ubd2=d2 ubd3=d3 ubd4=d4

# mdadm -C /dev/md0 --level=linear --raid-devices=4 /dev/ubd[1234]

adding some printks, I saw this:

[42949374.960000] hash_spacing = 821120
[42949374.960000] cnt          = 4
[42949374.960000] min_spacing  = 801
[42949374.960000] j=0 size=820928 sz=820928
[42949374.960000] i=0 sz=820928 hash_spacing=820928
[42949374.960000] j=1 size=64 sz=64
[42949374.960000] j=2 size=64 sz=128
[42949374.960000] j=3 size=64 sz=192
[42949374.960000] j=4 size=1515870810 sz=1515871002
drivers/md/linear.c +1 -1
Index: linus/drivers/md/linear.c
===================================================================
@@ -188,7 +188,7 @@
 	for (i=0; i < cnt-1 ; i++) {
 		sector_t sz = 0;
 		int j;
-		for (j=i; i<cnt-1 && sz < min_spacing ; j++)
+		for (j=i; j<cnt-1 && sz < min_spacing ; j++)
 			sz += conf->disks[j].size;
 		if (sz >= min_spacing && sz < conf->hash_spacing)
 			conf->hash_spacing = sz;
author
#1 lore raw
Quoted 27 lines · Andy Isaacson

When iterating through an array, one must be careful to test one's index variable rather than another similarly-named variable.

The loop will read off the end of conf->disks[] in the following (pathological) case:

% dd bs=1 seek=840716287 if=/dev/zero of=d1 count=1
% for i in 2 3 4; do dd if=/dev/zero of=d$i bs=1k count=$(($i+150)); done % ./vmlinux ubd0=root ubd1=d1 ubd2=d2 ubd3=d3 ubd4=d4

# mdadm -C /dev/md0 --level=linear --raid-devices=4 /dev/ubd[1234]

adding some printks, I saw this:

[42949374.960000] hash_spacing = 821120
[42949374.960000] cnt          = 4
[42949374.960000] min_spacing  = 801
[42949374.960000] j=0 size=820928 sz=820928
[42949374.960000] i=0 sz=820928 hash_spacing=820928
[42949374.960000] j=1 size=64 sz=64
[42949374.960000] j=2 size=64 sz=128
[42949374.960000] j=3 size=64 sz=192
[42949374.960000] j=4 size=1515870810 sz=1515871002

Index: linus/drivers/md/linear.c
===================================================================

drivers/md/linear.c line 194

Quoted 8 lines
drivers/md/linear.c +1 -1
@@ -188,7 +188,7 @@
 	for (i=0; i < cnt-1 ; i++) {
 		sector_t sz = 0;
 		int j;
-		for (j=i; i<cnt-1 && sz < min_spacing ; j++)
+		for (j=i; j<cnt-1 && sz < min_spacing ; j++)
 			sz += conf->disks[j].size;
 		if (sz >= min_spacing && sz < conf->hash_spacing)
 			conf->hash_spacing = sz;

Forgot to add:

Signed-off-by Andrew Isaacson <[email protected]>

-andy

Acked-by
#2 lore raw
Quoted 17 lines · On Thursday March 8, [email protected] wrote:

drivers/md/linear.c line 194

Quoted 13 lines · Andy Isaacson

Index: linus/drivers/md/linear.c
===================================================================

drivers/md/linear.c +1 -1
@@ -188,7 +188,7 @@
 	for (i=0; i < cnt-1 ; i++) {
 		sector_t sz = 0;
 		int j;
-		for (j=i; i<cnt-1 && sz < min_spacing ; j++)
+		for (j=i; j<cnt-1 && sz < min_spacing ; j++)
 			sz += conf->disks[j].size;
 		if (sz >= min_spacing && sz < conf->hash_spacing)
 			conf->hash_spacing = sz;

Forgot to add:

Quoted 1 line
Signed-off-by Andrew Isaacson <[email protected]>

And

Acked-by NeilBrown <[email protected]>

Thanks!

I would have replied earlier but I wanted to make sure I understood exactly what the possible consequences of this bug were.. and they are quite benign.
The worst possible outcome is going so far off the end of the array that you hit un-mapped memory and Oops.

If that doesn't happen, then the next worst option is that the hash table is sized poorly and you spend a few more cycles than needed choosing the target device for the request (we still always choose the right device).

Thanks,
NeilBrown

#3 lore raw
In reply to Andy Isaacson
Quoted 27 lines · Andy Isaacson

When iterating through an array, one must be careful to test one's index variable rather than another similarly-named variable.

The loop will read off the end of conf->disks[] in the following (pathological) case:

% dd bs=1 seek=840716287 if=/dev/zero of=d1 count=1
% for i in 2 3 4; do dd if=/dev/zero of=d$i bs=1k count=$(($i+150)); done % ./vmlinux ubd0=root ubd1=d1 ubd2=d2 ubd3=d3 ubd4=d4

# mdadm -C /dev/md0 --level=linear --raid-devices=4 /dev/ubd[1234]

adding some printks, I saw this:

[42949374.960000] hash_spacing = 821120
[42949374.960000] cnt          = 4
[42949374.960000] min_spacing  = 801
[42949374.960000] j=0 size=820928 sz=820928
[42949374.960000] i=0 sz=820928 hash_spacing=820928
[42949374.960000] j=1 size=64 sz=64
[42949374.960000] j=2 size=64 sz=128
[42949374.960000] j=3 size=64 sz=192
[42949374.960000] j=4 size=1515870810 sz=1515871002

Index: linus/drivers/md/linear.c
===================================================================

drivers/md/linear.c line 194

Quoted 8 lines
drivers/md/linear.c +1 -1
@@ -188,7 +188,7 @@
 	for (i=0; i < cnt-1 ; i++) {
 		sector_t sz = 0;
 		int j;
-		for (j=i; i<cnt-1 && sz < min_spacing ; j++)
+		for (j=i; j<cnt-1 && sz < min_spacing ; j++)
 			sz += conf->disks[j].size;
 		if (sz >= min_spacing && sz < conf->hash_spacing)
 			conf->hash_spacing = sz;

After looking at that code, I have to wonder how this ever worked, or if in fact anyone ever took this path. I assume that the value of sz caused the loop exit in all cases, since this has been in the code at least since 2.6.15, oldest thing I have handy.

Signature · 3 lines
bill davidsen <[email protected]>
 CTO TMR Associates, Inc
 Doing interesting things with small computers since 1979
author
#4 lore raw
Quoted 3 lines · Bill Davidsen
Quoted 2 lines · Andy Isaacson

% dd bs=1 seek=840716287 if=/dev/zero of=d1 count=1
% for i in 2 3 4; do dd if=/dev/zero of=d$i bs=1k count=$(($i+150)); done

[snip]

Quoted 8 lines
Quoted 3 lines
drivers/md/linear.c +1 -1
-		for (j=i; i<cnt-1 && sz < min_spacing ; j++)
+		for (j=i; j<cnt-1 && sz < min_spacing ; j++)

			sz += conf->disks[j].size;

After looking at that code, I have to wonder how this ever worked, or if in fact anyone ever took this path. I assume that the value of sz caused the loop exit in all cases, since this has been in the code at least since 2.6.15, oldest thing I have handy.

Well, just about any sane set of device sizes causes sz to rapidly exceed min_spacing. You'll notice that my failure case is { 800MB, 151kB, 152kB, 153kB, 154kB }.

And even in the failure case, it's just a read from uninitialized memory, which is probably either a small value (so it won't make the answer very wrong) or a large value (so it will be rejected in the immediately following code). In my case it happened to be some slab poison of 0xa5a5a5a5 or something like that, and the code went on just fine.

-andy