kobject: Read buffer overflow
PATCH 1/1 kobject: Read buffer overflow
Check whether index is within bounds before testing the element.
---
This also removes the likely, should it be kept?
drivers/parisc/pdc_stable.c +1 -1
@@ -370,7 +370,7 @@ pdcspath_layer_read(struct pdcspath_entry *entry, char *buf)if (!i)/* entry is not ready */ return -ENODATA ; -for (i =0 ; devpath->layers[i] && (likely (i <6 )); i++) +for (i =0 ; i <6 && devpath->layers[i]; i++) out +=sprintf (out,"%u " , devpath->layers[i]); out +=sprintf (out,"\n" );
On Sun, Aug 2, 2009 at 10:02 AM, Roel Kluin<[email protected]> wrote= :
Quoted 1 line
Check whether index is within bounds before testing the element.
The change is correct but:
- There are other places in the code with that construct. Even though they wouldn't trigger an overflow, why not fixing them too?
- Keep the likely: we are more likely to run out of data in the layers than to exhaust the counter (which is why no overflow was ever triggered, I believe ;-)
HTH
T-Bone
drivers/parisc/pdc_stable.=
Quoted 1 line
drivers/parisc/pdc_stable.= renamed from drivers/parisc/pdc_stable.c +0
-0
c
drivers/parisc/pdc_stable.c pdcspath_layer_read(struct pdcspath_entry *entry,=
Quoted 4 lines
index f9f9a5f..13a64bc 100644
drivers/parisc/pdc_stable.c +0
-0@@ -370,7 +370,7 @@ pdcspath_layer_read(struct pdcspath_entry *entry,=
char *buf)
Quoted 2 lines
=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!i) /* entry is not ready */ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENODAT=
A;
Quoted 2 lines
- =C2=A0 =C2=A0 =C2=A0 for (i =3D 0; devpath->layers[i] && (likely(i =
< 6)); i++)
Quoted 1 line
+ =C2=A0 =C2=A0 =C2=A0 for (i =3D 0; i < 6 && devpath->layers[i]; i++=
)
Quoted 1 line
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0out +=3D sprin=
tf(out, "%u ", devpath->layers[i]);
Quoted 4 lines
=C2=A0 =C2=A0 =C2=A0 =C2=A0out +=3D
sprintf(out, "\n");
--
To unsubscribe from this list: send the line "unsubscribe linux-paris=
c" in
Quoted 2 lines
the body of a message to [email protected]
More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.ht=
ml
Quoted 1 line
--=20
Thibaut VARENE
http://www.parisc-linux.org/~varenet/
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · Thibaut VARENE
Quoted 1 line · Roel Kluin
Check whether index is within bounds before testing the element.
The change is correct but:
- There are other places in the code with that construct. Even though they wouldn't trigger an overflow, why not fixing them too?
- Keep the likely: we are more likely to run out of data in the layers than to exhaust the counter (which is why no overflow was ever triggered, I believe ;-)
No, lose the likely. It's a for-loop; gcc will do the right thing.
(If you think I'm wrong, convince me by showing the disassembly of the compiled code with and without the likely).
Signature · 4 lines
Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 2 lines · Matthew Wilcox
Quoted 1 line · Thibaut VARENE
On Sun, Aug 2, 2009 at 10:02 AM, Roel Kluin<[email protected]> wr=
ote:
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Check whether index is within bounds before testing the element.
The change is correct but:
- There are other places in the code with that construct. Even thoug=
h
Quoted 2 lines
Quoted 2 lines
they wouldn't trigger an overflow, why not fixing them too?
- Keep the likely: we are more likely to run out of data in the laye=
rs
Quoted 4 lines
Quoted 2 lines
than to exhaust the counter (which is why no overflow was ever triggered, I believe ;-)
No, lose the likely. =C2=A0It's a for-loop; gcc will do the right thi=
ng.
I stand corrected then. ;-)
Thanks willy.
T-Bone
--=20
Thibaut VARENE
http://www.parisc-linux.org/~varenet/
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
James Bottomley
maintainerQuoted 11 lines · Roel Kluin
Check whether index is within bounds before testing the element.
Signed-off-by Roel Kluin <[email protected]>---
This also removes the likely, should it be kept?drivers/parisc/pdc_stable.c +0
-0
drivers/parisc/pdc_stable.c pdcspath_layer_read(struct pdcspath_entry *entry, char *buf) · line 373
Quoted 5 lines
drivers/parisc/pdc_stable.c +1
-1@@ -370,7 +370,7 @@ pdcspath_layer_read(struct pdcspath_entry *entry, char *buf)if (!i)/* entry is not ready */ return -ENODATA ; -for (i =0 ; devpath->layers[i] && (likely (i <6 )); i++) +for (i =0 ; i <6 && devpath->layers[i]; i++)
Since all patterns like this (swapping the order of conditions with no side effects in a for loop condition) are basically trivial, shouldn't they be going via Jiri Kosina (trivial tree)?
James