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 <[email protected]>
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);
}
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 <[email protected]>
>
> 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 <[email protected]>
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
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
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.
On Fri, Jul 23, 2010 at 10:48 AM, Joe Perches <[email protected]> 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
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
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