[patch] ath6kl: use a larger buffer for debug output

[patch] ath6kl: use a larger buffer for debug output

[Dan Carpenter]

This function makes the static checkers grumble.  The return value of
snprintf() is the number of bytes which would have been copied if
there was enough space.  In theory, a %u can take take 10 digits so
len could be larger than 16 and it would be a small information
leak.

We may as well make the buffer larger as well since that is very
easy to do.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 9eff0d0..e632008 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -1551,11 +1551,12 @@ static ssize_t ath6kl_listen_int_read(struct file *file,
 						size_t count, loff_t *ppos)
 {
 	struct ath6kl *ar = file->private_data;
-	char buf[16];
+	char buf[32];
 	int len;
 
 	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
 					ar->listen_intvl_b);
+	len = min(sizeof(buf), len);
 
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
 }

[patch] ath6kl: use a larger buffer for debug output

[Dan Carpenter]

This function makes the static checkers grumble.  The return value of
snprintf() is the number of bytes which would have been copied if
there was enough space.  In theory, a %u can take take 10 digits so
len could be larger than 16 and it would be a small information
leak.

We may as well make the buffer larger as well since that is very
easy to do.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 9eff0d0..e632008 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -1551,11 +1551,12 @@ static ssize_t ath6kl_listen_int_read(struct file *file,
 						size_t count, loff_t *ppos)
 {
 	struct ath6kl *ar = file->private_data;
-	char buf[16];
+	char buf[32];
 	int len;
 
 	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
 					ar->listen_intvl_b);
+	len = min(sizeof(buf), len);
 
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
 }

Re: [patch] ath6kl: use a larger buffer for debug output

[Johannes Berg]

On Fri, 2011-11-18 at 17:09 +0300, Dan Carpenter wrote:
> This function makes the static checkers grumble.  The return value of
> snprintf() is the number of bytes which would have been copied if
> there was enough space.  In theory, a %u can take take 10 digits so
> len could be larger than 16 and it would be a small information
> leak.
> 
> We may as well make the buffer larger as well since that is very
> easy to do.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
> index 9eff0d0..e632008 100644
> --- a/drivers/net/wireless/ath/ath6kl/debug.c
> +++ b/drivers/net/wireless/ath/ath6kl/debug.c
> @@ -1551,11 +1551,12 @@ static ssize_t ath6kl_listen_int_read(struct file *file,
>  						size_t count, loff_t *ppos)
>  {
>  	struct ath6kl *ar = file->private_data;
> -	char buf[16];
> +	char buf[32];
>  	int len;
>  
>  	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
>  					ar->listen_intvl_b);
> +	len = min(sizeof(buf), len);

Maybe that should be scnprintf instead then?

johannes

Re: [patch] ath6kl: use a larger buffer for debug output

[Johannes Berg]

On Fri, 2011-11-18 at 17:09 +0300, Dan Carpenter wrote:
> This function makes the static checkers grumble.  The return value of
> snprintf() is the number of bytes which would have been copied if
> there was enough space.  In theory, a %u can take take 10 digits so
> len could be larger than 16 and it would be a small information
> leak.
> 
> We may as well make the buffer larger as well since that is very
> easy to do.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
> index 9eff0d0..e632008 100644
> --- a/drivers/net/wireless/ath/ath6kl/debug.c
> +++ b/drivers/net/wireless/ath/ath6kl/debug.c
> @@ -1551,11 +1551,12 @@ static ssize_t ath6kl_listen_int_read(struct file *file,
>  						size_t count, loff_t *ppos)
>  {
>  	struct ath6kl *ar = file->private_data;
> -	char buf[16];
> +	char buf[32];
>  	int len;
>  
>  	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
>  					ar->listen_intvl_b);
> +	len = min(sizeof(buf), len);

Maybe that should be scnprintf instead then?

johannes

Re: [patch] ath6kl: use a larger buffer for debug output

[Kalle Valo]

On 11/18/2011 04:11 PM, Johannes Berg wrote:
> On Fri, 2011-11-18 at 17:09 +0300, Dan Carpenter wrote:
>> This function makes the static checkers grumble.  The return value of
>> snprintf() is the number of bytes which would have been copied if
>> there was enough space.  In theory, a %u can take take 10 digits so
>> len could be larger than 16 and it would be a small information
>> leak.
>>
>> We may as well make the buffer larger as well since that is very
>> easy to do.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
>> index 9eff0d0..e632008 100644
>> --- a/drivers/net/wireless/ath/ath6kl/debug.c
>> +++ b/drivers/net/wireless/ath/ath6kl/debug.c
>> @@ -1551,11 +1551,12 @@ static ssize_t ath6kl_listen_int_read(struct file *file,
>>  						size_t count, loff_t *ppos)
>>  {
>>  	struct ath6kl *ar = file->private_data;
>> -	char buf[16];
>> +	char buf[32];
>>  	int len;
>>  
>>  	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
>>  					ar->listen_intvl_b);
>> +	len = min(sizeof(buf), len);
> 
> Maybe that should be scnprintf instead then?

Yeah, I agree. And most likely debug.c has even more cases where
scnprintf() is needed.

Kalle

Re: [patch] ath6kl: use a larger buffer for debug output

[Kalle Valo]

On 11/18/2011 04:11 PM, Johannes Berg wrote:
> On Fri, 2011-11-18 at 17:09 +0300, Dan Carpenter wrote:
>> This function makes the static checkers grumble.  The return value of
>> snprintf() is the number of bytes which would have been copied if
>> there was enough space.  In theory, a %u can take take 10 digits so
>> len could be larger than 16 and it would be a small information
>> leak.
>>
>> We may as well make the buffer larger as well since that is very
>> easy to do.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
>> index 9eff0d0..e632008 100644
>> --- a/drivers/net/wireless/ath/ath6kl/debug.c
>> +++ b/drivers/net/wireless/ath/ath6kl/debug.c
>> @@ -1551,11 +1551,12 @@ static ssize_t ath6kl_listen_int_read(struct file *file,
>>  						size_t count, loff_t *ppos)
>>  {
>>  	struct ath6kl *ar = file->private_data;
>> -	char buf[16];
>> +	char buf[32];
>>  	int len;
>>  
>>  	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
>>  					ar->listen_intvl_b);
>> +	len = min(sizeof(buf), len);
> 
> Maybe that should be scnprintf instead then?

Yeah, I agree. And most likely debug.c has even more cases where
scnprintf() is needed.

Kalle

Re: [patch] ath6kl: use a larger buffer for debug output

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Mon, Nov 21, 2011 at 06:55:11PM +0200, Kalle Valo wrote:
> >>  	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
> >>  					ar->listen_intvl_b);
> >> +	len = min(sizeof(buf), len);
> > 
> > Maybe that should be scnprintf instead then?
> 
> Yeah, I agree. And most likely debug.c has even more cases where
> scnprintf() is needed.
> 

Yes yes.  Obviously.  I sent the patch at 5 pm on Friday and lost
track of it for a bit.  I'll send a fixed version tomorrow morning.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] ath6kl: use a larger buffer for debug output

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Mon, Nov 21, 2011 at 06:55:11PM +0200, Kalle Valo wrote:
> >>  	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
> >>  					ar->listen_intvl_b);
> >> +	len = min(sizeof(buf), len);
> > 
> > Maybe that should be scnprintf instead then?
> 
> Yeah, I agree. And most likely debug.c has even more cases where
> scnprintf() is needed.
> 

Yes yes.  Obviously.  I sent the patch at 5 pm on Friday and lost
track of it for a bit.  I'll send a fixed version tomorrow morning.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[patch v2] ath6kl: use a larger buffer for debug output

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

The return value of snprintf() is the number of bytes which would
have been copied if there was enough space, but we want the number of
bytes actually copied.  The scnprintf() function does this.

Also in theory, a %u can take take 10 digits so we may as well make
the buffer larger as well.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2:  I didn't know about the scnprintf() function before.  Thanks
Johannes.

diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 9eff0d0..eb808b4 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -1551,10 +1551,10 @@ static ssize_t ath6kl_listen_int_read(struct file *file,
 						size_t count, loff_t *ppos)
 {
 	struct ath6kl *ar = file->private_data;
-	char buf[16];
+	char buf[32];
 	int len;
 
-	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
+	len = scnprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
 					ar->listen_intvl_b);
 
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[patch v2] ath6kl: use a larger buffer for debug output

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

The return value of snprintf() is the number of bytes which would
have been copied if there was enough space, but we want the number of
bytes actually copied.  The scnprintf() function does this.

Also in theory, a %u can take take 10 digits so we may as well make
the buffer larger as well.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2:  I didn't know about the scnprintf() function before.  Thanks
Johannes.

diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 9eff0d0..eb808b4 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -1551,10 +1551,10 @@ static ssize_t ath6kl_listen_int_read(struct file *file,
 						size_t count, loff_t *ppos)
 {
 	struct ath6kl *ar = file->private_data;
-	char buf[16];
+	char buf[32];
 	int len;
 
-	len = snprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
+	len = scnprintf(buf, sizeof(buf), "%u %u\n", ar->listen_intvl_t,
 					ar->listen_intvl_b);
 
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch v2] ath6kl: use a larger buffer for debug output

[Kalle Valo]

On 11/23/2011 08:34 AM, Dan Carpenter wrote:
> The return value of snprintf() is the number of bytes which would
> have been copied if there was enough space, but we want the number of
> bytes actually copied.  The scnprintf() function does this.
> 
> Also in theory, a %u can take take 10 digits so we may as well make
> the buffer larger as well.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks, applied.

> v2:  I didn't know about the scnprintf() function before.  Thanks
> Johannes.

Me neither, so thank you from me as well :)

Kalle

Re: [patch v2] ath6kl: use a larger buffer for debug output

[Kalle Valo]

On 11/23/2011 08:34 AM, Dan Carpenter wrote:
> The return value of snprintf() is the number of bytes which would
> have been copied if there was enough space, but we want the number of
> bytes actually copied.  The scnprintf() function does this.
> 
> Also in theory, a %u can take take 10 digits so we may as well make
> the buffer larger as well.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks, applied.

> v2:  I didn't know about the scnprintf() function before.  Thanks
> Johannes.

Me neither, so thank you from me as well :)

Kalle