[patch -next] ath5k: snprintf() returns largish values

[patch -next] ath5k: snprintf() returns largish values

[Dan Carpenter]

snprintf() returns the number of characters that would have been written
(not counting the NUL character).  So we can't use it as the limiter to 
simple_read_from_buffer() without capping it first at sizeof(buf).

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c
index ebb9c23..4cccc29 100644
--- a/drivers/net/wireless/ath/ath5k/debug.c
+++ b/drivers/net/wireless/ath/ath5k/debug.c
@@ -239,6 +239,9 @@ static ssize_t read_file_beacon(struct file *file, char __user *user_buf,
 		"TSF\t\t0x%016llx\tTU: %08x\n",
 		(unsigned long long)tsf, TSF_TO_TU(tsf));
 
+	if (len > sizeof(buf))
+		len = sizeof(buf);
+
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
 }
 
@@ -334,6 +337,9 @@ static ssize_t read_file_debug(struct file *file, char __user *user_buf,
 		sc->debug.level == dbg_info[i].level ? '+' : ' ',
 		dbg_info[i].level, dbg_info[i].desc);
 
+	if (len > sizeof(buf))
+		len = sizeof(buf);
+
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
 }
 
@@ -433,6 +439,9 @@ static ssize_t read_file_antenna(struct file *file, char __user *user_buf,
 	len += snprintf(buf+len, sizeof(buf)-len,
 			"AR5K_PHY_ANT_SWITCH_TABLE_1\t0x%08x\n", v);
 
+	if (len > sizeof(buf))
+		len = sizeof(buf);
+
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
 }
 
@@ -542,6 +551,9 @@ static ssize_t read_file_frameerrors(struct file *file, char __user *user_buf,
 	len += snprintf(buf+len, sizeof(buf)-len, "[TX all\t%d]\n",
 			st->tx_all_count);
 
+	if (len > sizeof(buf))
+		len = sizeof(buf);
+
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
 }
 
@@ -681,6 +693,9 @@ static ssize_t read_file_ani(struct file *file, char __user *user_buf,
 			ATH5K_ANI_CCK_TRIG_HIGH - (ATH5K_PHYERR_CNT_MAX -
 			ath5k_hw_reg_read(sc->ah, AR5K_PHYERR_CNT2)));
 
+	if (len > sizeof(buf))
+		len = sizeof(buf);
+
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
 }
 
@@ -766,6 +781,9 @@ static ssize_t read_file_queue(struct file *file, char __user *user_buf,
 		len += snprintf(buf+len, sizeof(buf)-len, "  len: %d\n", n);
 	}
 
+	if (len > sizeof(buf))
+		len = sizeof(buf);
+
 	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
 }
 

Re: [patch -next] ath5k: snprintf() returns largish values

[Jiri Slaby]

On 07/22/2010 10:52 AM, Dan Carpenter wrote:
> snprintf() returns the number of characters that would have been written
> (not counting the NUL character).  So we can't use it as the limiter to 
> simple_read_from_buffer() without capping it first at sizeof(buf).

Doesn't scnprintf make more sense here?

thanks,
-- 
js

Re: [patch -next] ath5k: snprintf() returns largish values

[Dan Carpenter]

On Thu, Jul 22, 2010 at 10:56:13AM +0200, Jiri Slaby wrote:
> On 07/22/2010 10:52 AM, Dan Carpenter wrote:
> > snprintf() returns the number of characters that would have been written
> > (not counting the NUL character).  So we can't use it as the limiter to 
> > simple_read_from_buffer() without capping it first at sizeof(buf).
> 
> Doesn't scnprintf make more sense here?
> 

Not really...  It's nice to pass a negative number as the buffer size to
snprintf() instead of having to make that a special case.

regards,
dan carpenter

> thanks,
> -- 
> js

Re: [patch -next] ath5k: snprintf() returns largish values

[Bruno Randolf]

On Thu July 22 2010 17:52:02 Dan Carpenter wrote:
> snprintf() returns the number of characters that would have been written
> (not counting the NUL character).  So we can't use it as the limiter to
> simple_read_from_buffer() without capping it first at sizeof(buf).
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/drivers/net/wireless/ath/ath5k/debug.c
> b/drivers/net/wireless/ath/ath5k/debug.c index ebb9c23..4cccc29 100644
> --- a/drivers/net/wireless/ath/ath5k/debug.c
> +++ b/drivers/net/wireless/ath/ath5k/debug.c
> @@ -239,6 +239,9 @@ static ssize_t read_file_beacon(struct file *file, char
> __user *user_buf, "TSF\t\t0x%016llx\tTU: %08x\n",
>  		(unsigned long long)tsf, TSF_TO_TU(tsf));
> 
> +	if (len > sizeof(buf))
> +		len = sizeof(buf);
> +
>  	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>  }
> 
> @@ -334,6 +337,9 @@ static ssize_t read_file_debug(struct file *file, char
> __user *user_buf, sc->debug.level == dbg_info[i].level ? '+' : ' ',
>  		dbg_info[i].level, dbg_info[i].desc);
> 
> +	if (len > sizeof(buf))
> +		len = sizeof(buf);
> +
>  	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>  }
> 
> @@ -433,6 +439,9 @@ static ssize_t read_file_antenna(struct file *file,
> char __user *user_buf, len += snprintf(buf+len, sizeof(buf)-len,
>  			"AR5K_PHY_ANT_SWITCH_TABLE_1\t0x%08x\n", v);
> 
> +	if (len > sizeof(buf))
> +		len = sizeof(buf);
> +
>  	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>  }
> 
> @@ -542,6 +551,9 @@ static ssize_t read_file_frameerrors(struct file *file,
> char __user *user_buf, len += snprintf(buf+len, sizeof(buf)-len, "[TX
> all\t%d]\n",
>  			st->tx_all_count);
> 
> +	if (len > sizeof(buf))
> +		len = sizeof(buf);
> +
>  	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>  }
> 
> @@ -681,6 +693,9 @@ static ssize_t read_file_ani(struct file *file, char
> __user *user_buf, ATH5K_ANI_CCK_TRIG_HIGH - (ATH5K_PHYERR_CNT_MAX -
>  			ath5k_hw_reg_read(sc->ah, AR5K_PHYERR_CNT2)));
> 
> +	if (len > sizeof(buf))
> +		len = sizeof(buf);
> +
>  	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>  }
> 
> @@ -766,6 +781,9 @@ static ssize_t read_file_queue(struct file *file, char
> __user *user_buf, len += snprintf(buf+len, sizeof(buf)-len, "  len: %d\n",
> n);
>  	}
> 
> +	if (len > sizeof(buf))
> +		len = sizeof(buf);
> +
>  	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>  }

i think it would be better to make sure the buffer is always big enough to 
hold all the output (it's not very variable in length), but as a safety net 
this can't hurt.

Acked-by: Bruno Randolf <br1@einfach.org>

Re: [patch -next] ath5k: snprintf() returns largish values

[Dan Carpenter]

On Fri, Jul 23, 2010 at 05:44:14PM +0900, Bruno Randolf wrote:
> 
> i think it would be better to make sure the buffer is always big enough to 
> hold all the output (it's not very variable in length), but as a safety net 
> this can't hurt.
> 

This is a smatch thing.  I suppose someday I will fix smatch to
evaulate the strings themselves and verify that the buffer is large
enough.  But for now it's nice to be able to automatically check that
the buffers don't overflow.  

regards,
dan carpenter

Re: [patch -next] ath5k: snprintf() returns largish values

[Joe Perches]

On Fri, 2010-07-23 at 12:04 +0200, Dan Carpenter wrote:
> This is a smatch thing.  I suppose someday I will fix smatch to
> evaulate the strings themselves and verify that the buffer is large
> enough.  But for now it's nice to be able to automatically check that
> the buffers don't overflow.  

There are also many repeated uses of snprintf in kernel sources
that could similarly be a problem.

	bar += snprintf(foo + bar, ...)
	bar += snprintf(foo + bar, ...)
or
	foo += snprintf(foo, ...)
	foo += snprintf(foo, ...)

For instance:

$ grep -P -n -A 4 -m 3 "\+=\s*snprintf" drivers/net/wireless/ath/ath5k/debug.c
210:	len += snprintf(buf+len, sizeof(buf)-len,
211-		"%-24s0x%08x\tintval: %d\tTIM: 0x%x\n",
212-		"AR5K_BEACON", v, v & AR5K_BEACON_PERIOD,
213-		(v & AR5K_BEACON_TIM) >> AR5K_BEACON_TIM_S);
214-
215:	len += snprintf(buf+len, sizeof(buf)-len, "%-24s0x%08x\n",
216-		"AR5K_LAST_TSTP", ath5k_hw_reg_read(sc->ah, AR5K_LAST_TSTP));
217-
218:	len += snprintf(buf+len, sizeof(buf)-len, "%-24s0x%08x\n\n",
219-		"AR5K_BEACON_CNT", ath5k_hw_reg_read(sc->ah, AR5K_BEACON_CNT));
220-

A conversion from snprintf to scnprintf might be appropriate
for those patterns.

Re: [patch -next] ath5k: snprintf() returns largish values

[Linus Torvalds]

On Fri, Jul 23, 2010 at 10:48 AM, Joe Perches <joe@perches.com> wrote:
>
> There are also many repeated uses of snprintf in kernel sources
> that could similarly be a problem.
>
>        bar += snprintf(foo + bar, ...)
>        bar += snprintf(foo + bar, ...)
> or
>        foo += snprintf(foo, ...)
>        foo += snprintf(foo, ...)

As long as the number of bytes is updated correctly, this won't be a
security problem, although it can cause a (single) warning. The kernel
vsnprintf does

        if (WARN_ON_ONCE((int) size < 0))
                return 0;

so if somebody overflows a buffer with multiple snprintf calls, it
will all be ok as long as the buffer size thing is updated the natural
way (possibly using pointer arithmetic, eg "end - bar").

                Linus

Re: [patch -next] ath5k: snprintf() returns largish values

[walter harms]

Bruno Randolf schrieb:

>>
>> @@ -766,6 +781,9 @@ static ssize_t read_file_queue(struct file *file, char
>> __user *user_buf, len += snprintf(buf+len, sizeof(buf)-len, "  len: %d\n",
>> n);
>>  	}
>>
>> +	if (len > sizeof(buf))
>> +		len = sizeof(buf);
>> +
>>  	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>>  }
> 
> i think it would be better to make sure the buffer is always big enough to 
> hold all the output (it's not very variable in length), but as a safety net 
> this can't hurt.
> 


glibc provides open_memstream()/fmemopen() to write into large buffers.
I feel this as a better way than  len += snprintf(buf+len)

re,
 wh