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

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

[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
===================================================================
--- linus.orig/drivers/md/linear.c	2007-03-02 11:35:55.000000000 -0800
+++ linus/drivers/md/linear.c	2007-03-07 13:10:30.000000000 -0800
@@ -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;

Re: [PATCH] fix read past end of array in md/linear.c

[Bill Davidsen]

Andy Isaacson wrote:
> 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
> ===================================================================
> --- linus.orig/drivers/md/linear.c	2007-03-02 11:35:55.000000000 -0800
> +++ linus/drivers/md/linear.c	2007-03-07 13:10:30.000000000 -0800
> @@ -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.

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979

Re: [PATCH] fix read past end of array in md/linear.c

[Andy Isaacson]

On Thu, Mar 08, 2007 at 09:37:46PM -0500, Bill Davidsen wrote:
> Andy Isaacson wrote:
> >% 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]
> >-		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