ath5k: snprintf() returns largish values

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

Quoted 3 lines · 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).

Doesn't scnprintf make more sense here?

thanks,

Signature · 1 line

js

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]>

(quoted diff) +0 -0

b/drivers/net/wireless/ath/ath5k/debug.c index ebb9c23..4cccc29 100644

drivers/net/wireless/ath/ath5k/debug.c +9 -0

@@ -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",

drivers/net/wireless/ath/ath5k/debug.c +3 -0

 			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 -

drivers/net/wireless/ath/ath5k/debug.c +3 -0

 			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);
 }

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.

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, ...)

Quoted 6 lines

(quoted diff) +0 -0

@@ -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);
 	}
 

drivers/net/wireless/ath/ath5k/debug.c static ssize_t read_file_beacon(struct file *file, char __user *user_buf, · line 246

Quoted 5 lines

drivers/net/wireless/ath/ath5k/debug.c +3 -0

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