[PATCH] IIO: Documentation: iio_utils: Prevent buffer overflow

[PATCH] IIO: Documentation: iio_utils: Prevent buffer overflow

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

The first part of build_channel_array()identifies the number of enabled channels.
Further down this count is used to allocate the ci_array. The next section parses the
scan_elements directory again, and fills ci_array regardless if the channel is enabled or not.
So if less than available channels are enabled ci_array memory is overflowed.

This fix makes sure that we allocate enough memory. But the whole approach looks a bit
cumbersome to me. Why not allocate memory for MAX_CHANNLES, less say 64
(I never seen a part with more than that channels). And skip the first part entirely.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/staging/iio/Documentation/iio_utils.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/Documentation/iio_utils.h b/drivers/staging/iio/Documentation/iio_utils.h
index 4b023aa..bde2313 100644
--- a/drivers/staging/iio/Documentation/iio_utils.h
+++ b/drivers/staging/iio/Documentation/iio_utils.h
@@ -290,15 +290,17 @@ inline int build_channel_array(const char *device_dir,
 			fscanf(sysfsfp, "%u", &ret);
 			if (ret == 1)
 				(*counter)++;
+			count++;
 			fclose(sysfsfp);
 			free(filename);
 		}
-	*ci_array = malloc(sizeof(**ci_array)*(*counter));
+	*ci_array = malloc(sizeof(**ci_array)*count);
 	if (*ci_array == NULL) {
 		ret = -ENOMEM;
 		goto error_close_dir;
 	}
 	seekdir(dp, 0);
+	count = 0;
 	while (ent = readdir(dp), ent != NULL) {
 		if (strcmp(ent->d_name + strlen(ent->d_name) - strlen("_en"),
 			   "_en") == 0) {
-- 
1.6.0.2

Re: [PATCH] IIO: Documentation: iio_utils: Prevent buffer overflow

[Jonathan Cameron]

On 02/16/11 20:16, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> The first part of build_channel_array()identifies the number of enabled channels.
> Further down this count is used to allocate the ci_array. The next section parses the
> scan_elements directory again, and fills ci_array regardless if the channel is enabled or not.
> So if less than available channels are enabled ci_array memory is overflowed.
Good point. Oops...  I guess all my test cases actually had all channels enabled.
> 
> This fix makes sure that we allocate enough memory. But the whole approach looks a bit
> cumbersome to me. Why not allocate memory for MAX_CHANNLES, less say 64
> (I never seen a part with more than that channels). And skip the first part entirely.
Could do, but I'd rather keep this fully general and it's only slightly cumbersome.
Probably better ways of writing this whole function though now I think about it...
Perhaps some scandir magic as could get that to give a sorted list of _en
attribute names saving the sorting of the array at the end.
> 
Anyhow, definitely send this fix on!
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/Documentation/iio_utils.h |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/iio/Documentation/iio_utils.h b/drivers/staging/iio/Documentation/iio_utils.h
> index 4b023aa..bde2313 100644
> --- a/drivers/staging/iio/Documentation/iio_utils.h
> +++ b/drivers/staging/iio/Documentation/iio_utils.h
> @@ -290,15 +290,17 @@ inline int build_channel_array(const char *device_dir,
>  			fscanf(sysfsfp, "%u", &ret);
>  			if (ret == 1)
>  				(*counter)++;
> +			count++;
>  			fclose(sysfsfp);
>  			free(filename);
>  		}
> -	*ci_array = malloc(sizeof(**ci_array)*(*counter));
> +	*ci_array = malloc(sizeof(**ci_array)*count);
>  	if (*ci_array == NULL) {
>  		ret = -ENOMEM;
>  		goto error_close_dir;
>  	}
>  	seekdir(dp, 0);
> +	count = 0;
>  	while (ent = readdir(dp), ent != NULL) {
>  		if (strcmp(ent->d_name + strlen(ent->d_name) - strlen("_en"),
>  			   "_en") == 0) {

Re: [PATCH] IIO: Documentation: iio_utils: Prevent buffer overflow

[Michael Hennerich]

On 02/22/2011 08:29 PM, Jonathan Cameron wrote:
> On 02/16/11 20:16, michael.hennerich@analog.com wrote:
>   
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> The first part of build_channel_array()identifies the number of enabled channels.
>> Further down this count is used to allocate the ci_array. The next section parses the
>> scan_elements directory again, and fills ci_array regardless if the channel is enabled or not.
>> So if less than available channels are enabled ci_array memory is overflowed.
>>     
> Good point. Oops...  I guess all my test cases actually had all channels enabled.
>   
>> This fix makes sure that we allocate enough memory. But the whole approach looks a bit
>> cumbersome to me. Why not allocate memory for MAX_CHANNLES, less say 64
>> (I never seen a part with more than that channels). And skip the first part entirely.
>>     
> Could do, but I'd rather keep this fully general and it's only slightly cumbersome.
> Probably better ways of writing this whole function though now I think about it...
> Perhaps some scandir magic as could get that to give a sorted list of _en
> attribute names saving the sorting of the array at the end.
>   
>>     
> Anyhow, definitely send this fix on!
>   
Given the fact that this is loose user space example code under the
Documentation folder.
- Does it really need to go into stable?
Same question applies to the other none style fixes to your example code.

>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>     
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>   
>> ---
>>  drivers/staging/iio/Documentation/iio_utils.h |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/staging/iio/Documentation/iio_utils.h b/drivers/staging/iio/Documentation/iio_utils.h
>> index 4b023aa..bde2313 100644
>> --- a/drivers/staging/iio/Documentation/iio_utils.h
>> +++ b/drivers/staging/iio/Documentation/iio_utils.h
>> @@ -290,15 +290,17 @@ inline int build_channel_array(const char *device_dir,
>>                       fscanf(sysfsfp, "%u", &ret);
>>                       if (ret == 1)
>>                               (*counter)++;
>> +                     count++;
>>                       fclose(sysfsfp);
>>                       free(filename);
>>               }
>> -     *ci_array = malloc(sizeof(**ci_array)*(*counter));
>> +     *ci_array = malloc(sizeof(**ci_array)*count);
>>       if (*ci_array == NULL) {
>>               ret = -ENOMEM;
>>               goto error_close_dir;
>>       }
>>       seekdir(dp, 0);
>> +     count = 0;
>>       while (ent = readdir(dp), ent != NULL) {
>>               if (strcmp(ent->d_name + strlen(ent->d_name) - strlen("_en"),
>>                          "_en") == 0) {
>>     
>   


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif

Re: [PATCH] IIO: Documentation: iio_utils: Prevent buffer overflow

[Jonathan Cameron]

On 02/22/11 20:50, Michael Hennerich wrote:
> On 02/22/2011 08:29 PM, Jonathan Cameron wrote:
>> On 02/16/11 20:16, michael.hennerich@analog.com wrote:
>>   
>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>
>>> The first part of build_channel_array()identifies the number of enabled channels.
>>> Further down this count is used to allocate the ci_array. The next section parses the
>>> scan_elements directory again, and fills ci_array regardless if the channel is enabled or not.
>>> So if less than available channels are enabled ci_array memory is overflowed.
>>>     
>> Good point. Oops...  I guess all my test cases actually had all channels enabled.
>>   
>>> This fix makes sure that we allocate enough memory. But the whole approach looks a bit
>>> cumbersome to me. Why not allocate memory for MAX_CHANNLES, less say 64
>>> (I never seen a part with more than that channels). And skip the first part entirely.
>>>     
>> Could do, but I'd rather keep this fully general and it's only slightly cumbersome.
>> Probably better ways of writing this whole function though now I think about it...
>> Perhaps some scandir magic as could get that to give a sorted list of _en
>> attribute names saving the sorting of the array at the end.
>>   
>>>     
>> Anyhow, definitely send this fix on!
>>   
> Given the fact that this is loose user space example code under the
> Documentation folder.
> - Does it really need to go into stable?
Certainly not as important if it were in a driver. Lets not bother unless
someone else picks up on it.
> Same question applies to the other none style fixes to your example code.
> 
>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>     
>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>>   
>>> ---
>>>  drivers/staging/iio/Documentation/iio_utils.h |    4 +++-
>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/Documentation/iio_utils.h b/drivers/staging/iio/Documentation/iio_utils.h
>>> index 4b023aa..bde2313 100644
>>> --- a/drivers/staging/iio/Documentation/iio_utils.h
>>> +++ b/drivers/staging/iio/Documentation/iio_utils.h
>>> @@ -290,15 +290,17 @@ inline int build_channel_array(const char *device_dir,
>>>                       fscanf(sysfsfp, "%u", &ret);
>>>                       if (ret == 1)
>>>                               (*counter)++;
>>> +                     count++;
>>>                       fclose(sysfsfp);
>>>                       free(filename);
>>>               }
>>> -     *ci_array = malloc(sizeof(**ci_array)*(*counter));
>>> +     *ci_array = malloc(sizeof(**ci_array)*count);
>>>       if (*ci_array == NULL) {
>>>               ret = -ENOMEM;
>>>               goto error_close_dir;
>>>       }
>>>       seekdir(dp, 0);
>>> +     count = 0;
>>>       while (ent = readdir(dp), ent != NULL) {
>>>               if (strcmp(ent->d_name + strlen(ent->d_name) - strlen("_en"),
>>>                          "_en") == 0) {
>>>     
>>   
> 
>