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

2007-03-11 Thread Neil Brown 
On Thursday March 8, [EMAIL PROTECTED] wrote:
> On Thu, Mar 08, 2007 at 12:52:04PM -0800, Andy Isaacson wrote:
> > Index: linus/drivers/md/linear.c
> > ===
> > --- linus.orig/drivers/md/linear.c  2007-03-02 11:35:55.0 -0800
> > +++ linus/drivers/md/linear.c   2007-03-07 13:10:30.0 -0800
> > @@ -188,7 +188,7 @@
> > for (i=0; i < cnt-1 ; i++) {
> > sector_t sz = 0;
> > int j;
> > -   for (j=i; i > +   for (j=i; 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]>

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2007-03-11 Thread Neil Brown 
On Thursday March 8, [EMAIL PROTECTED] wrote:
 On Thu, Mar 08, 2007 at 12:52:04PM -0800, Andy Isaacson wrote:
  Index: linus/drivers/md/linear.c
  ===
  --- linus.orig/drivers/md/linear.c  2007-03-02 11:35:55.0 -0800
  +++ linus/drivers/md/linear.c   2007-03-07 13:10:30.0 -0800
  @@ -188,7 +188,7 @@
  for (i=0; i  cnt-1 ; i++) {
  sector_t sz = 0;
  int j;
  -   for (j=i; icnt-1  sz  min_spacing ; j++)
  +   for (j=i; jcnt-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]

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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2007-03-08 Thread 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 >+for (j=i; 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2007-03-08 Thread 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.96] hash_spacing = 821120
[42949374.96] cnt  = 4
[42949374.96] min_spacing  = 801
[42949374.96] j=0 size=820928 sz=820928
[42949374.96] i=0 sz=820928 hash_spacing=820928
[42949374.96] j=1 size=64 sz=64
[42949374.96] j=2 size=64 sz=128
[42949374.96] j=3 size=64 sz=192
[42949374.96] j=4 size=1515870810 sz=1515871002

Index: linus/drivers/md/linear.c
===
--- linus.orig/drivers/md/linear.c  2007-03-02 11:35:55.0 -0800
+++ linus/drivers/md/linear.c   2007-03-07 13:10:30.0 -0800
@@ -188,7 +188,7 @@
for (i=0; i < cnt-1 ; i++) {
sector_t sz = 0;
int j;
-   for (j=i; idisks[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 <[EMAIL PROTECTED]>
 CTO TMR Associates, Inc
 Doing interesting things with small computers since 1979

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2007-03-08 Thread Andy Isaacson 
On Thu, Mar 08, 2007 at 12:52:04PM -0800, 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.96] hash_spacing = 821120
> [42949374.96] cnt  = 4
> [42949374.96] min_spacing  = 801
> [42949374.96] j=0 size=820928 sz=820928
> [42949374.96] i=0 sz=820928 hash_spacing=820928
> [42949374.96] j=1 size=64 sz=64
> [42949374.96] j=2 size=64 sz=128
> [42949374.96] j=3 size=64 sz=192
> [42949374.96] j=4 size=1515870810 sz=1515871002
> 
> Index: linus/drivers/md/linear.c
> ===
> --- linus.orig/drivers/md/linear.c2007-03-02 11:35:55.0 -0800
> +++ linus/drivers/md/linear.c 2007-03-07 13:10:30.0 -0800
> @@ -188,7 +188,7 @@
>   for (i=0; i < cnt-1 ; i++) {
>   sector_t sz = 0;
>   int j;
> - for (j=i; i + for (j=i; 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2007-03-08 Thread 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.96] hash_spacing = 821120
[42949374.96] cnt  = 4
[42949374.96] min_spacing  = 801
[42949374.96] j=0 size=820928 sz=820928
[42949374.96] i=0 sz=820928 hash_spacing=820928
[42949374.96] j=1 size=64 sz=64
[42949374.96] j=2 size=64 sz=128
[42949374.96] j=3 size=64 sz=192
[42949374.96] j=4 size=1515870810 sz=1515871002

Index: linus/drivers/md/linear.c
===
--- linus.orig/drivers/md/linear.c  2007-03-02 11:35:55.0 -0800
+++ linus/drivers/md/linear.c   2007-03-07 13:10:30.0 -0800
@@ -188,7 +188,7 @@
for (i=0; i < cnt-1 ; i++) {
sector_t sz = 0;
int j;
-   for (j=i; idisks[j].size;
if (sz >= min_spacing && sz < conf->hash_spacing)
conf->hash_spacing = sz;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2007-03-08 Thread Andy Isaacson 
On Thu, Mar 08, 2007 at 12:52:04PM -0800, 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.96] hash_spacing = 821120
 [42949374.96] cnt  = 4
 [42949374.96] min_spacing  = 801
 [42949374.96] j=0 size=820928 sz=820928
 [42949374.96] i=0 sz=820928 hash_spacing=820928
 [42949374.96] j=1 size=64 sz=64
 [42949374.96] j=2 size=64 sz=128
 [42949374.96] j=3 size=64 sz=192
 [42949374.96] j=4 size=1515870810 sz=1515871002
 
 Index: linus/drivers/md/linear.c
 ===
 --- linus.orig/drivers/md/linear.c2007-03-02 11:35:55.0 -0800
 +++ linus/drivers/md/linear.c 2007-03-07 13:10:30.0 -0800
 @@ -188,7 +188,7 @@
   for (i=0; i  cnt-1 ; i++) {
   sector_t sz = 0;
   int j;
 - for (j=i; icnt-1  sz  min_spacing ; j++)
 + for (j=i; jcnt-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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2007-03-08 Thread 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.96] hash_spacing = 821120
[42949374.96] cnt  = 4
[42949374.96] min_spacing  = 801
[42949374.96] j=0 size=820928 sz=820928
[42949374.96] i=0 sz=820928 hash_spacing=820928
[42949374.96] j=1 size=64 sz=64
[42949374.96] j=2 size=64 sz=128
[42949374.96] j=3 size=64 sz=192
[42949374.96] j=4 size=1515870810 sz=1515871002

Index: linus/drivers/md/linear.c
===
--- linus.orig/drivers/md/linear.c  2007-03-02 11:35:55.0 -0800
+++ linus/drivers/md/linear.c   2007-03-07 13:10:30.0 -0800
@@ -188,7 +188,7 @@
for (i=0; i  cnt-1 ; i++) {
sector_t sz = 0;
int j;
-   for (j=i; icnt-1  sz  min_spacing ; j++)
+   for (j=i; jcnt-1  sz  min_spacing ; j++)
sz += conf-disks[j].size;
if (sz = min_spacing  sz  conf-hash_spacing)
conf-hash_spacing = sz;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2007-03-08 Thread 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.96] hash_spacing = 821120
[42949374.96] cnt  = 4
[42949374.96] min_spacing  = 801
[42949374.96] j=0 size=820928 sz=820928
[42949374.96] i=0 sz=820928 hash_spacing=820928
[42949374.96] j=1 size=64 sz=64
[42949374.96] j=2 size=64 sz=128
[42949374.96] j=3 size=64 sz=192
[42949374.96] j=4 size=1515870810 sz=1515871002

Index: linus/drivers/md/linear.c
===
--- linus.orig/drivers/md/linear.c  2007-03-02 11:35:55.0 -0800
+++ linus/drivers/md/linear.c   2007-03-07 13:10:30.0 -0800
@@ -188,7 +188,7 @@
for (i=0; i  cnt-1 ; i++) {
sector_t sz = 0;
int j;
-   for (j=i; icnt-1  sz  min_spacing ; j++)
+   for (j=i; jcnt-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 [EMAIL PROTECTED]
 CTO TMR Associates, Inc
 Doing interesting things with small computers since 1979

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2007-03-08 Thread 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; icnt-1  sz  min_spacing ; j++)
 +for (j=i; jcnt-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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/