[PATCH] kobject: Read buffer overflow

[PATCH] kobject: Read buffer overflow

[Roel Kluin]

Check whether index is within bounds before testing the element.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
This also removes the likely, should it be kept?

diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c
index f9f9a5f..13a64bc 100644
--- a/drivers/parisc/pdc_stable.c
+++ b/drivers/parisc/pdc_stable.c
@@ -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");

Re: [PATCH] kobject: Read buffer overflow

[Thibaut VARENE]

On Sun, Aug 2, 2009 at 10:02 AM, Roel Kluin<roel.kluin@gmail.com> wrote=
:
> 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

> diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.=
c
> index f9f9a5f..13a64bc 100644
> --- a/drivers/parisc/pdc_stable.c
> +++ b/drivers/parisc/pdc_stable.c
> @@ -370,7 +370,7 @@ pdcspath_layer_read(struct pdcspath_entry *entry,=
 char *buf)
> =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;
>
> - =C2=A0 =C2=A0 =C2=A0 for (i =3D 0; devpath->layers[i] && (likely(i =
< 6)); i++)
> + =C2=A0 =C2=A0 =C2=A0 for (i =3D 0; i < 6 && devpath->layers[i]; i++=
)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0out +=3D sprin=
tf(out, "%u ", devpath->layers[i]);
>
> =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
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.ht=
ml
>



--=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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kobject: Read buffer overflow

[Matthew Wilcox]

On Sun, Aug 02, 2009 at 12:06:59PM +0200, Thibaut VARENE wrote:
> On Sun, Aug 2, 2009 at 10:02 AM, Roel Kluin<roel.kluin@gmail.com> wrote:
> > 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).

-- 
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."

Re: [PATCH] kobject: Read buffer overflow

[Thibaut VARENE]

On Sun, Aug 2, 2009 at 12:16 PM, Matthew Wilcox<matthew@wil.cx> wrote:
> On Sun, Aug 02, 2009 at 12:06:59PM +0200, Thibaut VARENE wrote:
>> On Sun, Aug 2, 2009 at 10:02 AM, Roel Kluin<roel.kluin@gmail.com> wr=
ote:
>> > 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
>> 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
>> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kobject: Read buffer overflow

[James Bottomley]

On Sun, 2009-08-02 at 10:02 +0200, Roel Kluin wrote:
> Check whether index is within bounds before testing the element.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> This also removes the likely, should it be kept?
> 
> diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c
> index f9f9a5f..13a64bc 100644
> --- a/drivers/parisc/pdc_stable.c
> +++ b/drivers/parisc/pdc_stable.c
> @@ -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