drivers/net/wireless/ipw2x00: use strlcpy instead of strncpy

The fields must be null-terminated, or IPW_DEBUG_ASSOC will cause issue.

drivers/net/wireless/ipw2x00/ipw2200.c +2 -2

@@ -5558,7 +5558,7 @@ static int ipw_find_adhoc_network(struct ipw_priv *priv,
 			    min(network->ssid_len, priv->essid_len)))) {
 			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
 
-			strncpy(escaped,
+			strlcpy(escaped,
 				print_ssid(ssid, network->ssid,
 					   network->ssid_len),
 				sizeof(escaped));

@@ -5771,7 +5771,7 @@ static int ipw_best_network(struct ipw_priv *priv,
 		     memcmp(network->ssid, priv->essid,
 			    min(network->ssid_len, priv->essid_len)))) {
 			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
-			strncpy(escaped,
+			strlcpy(escaped,
 				print_ssid(ssid, network->ssid,
 					   network->ssid_len),
 				sizeof(escaped));

This happens because escaped is declared the wrong size.

It'd be better to change

	char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
to
	DECLARE_SSID_BUF(escaped);
and use
	print_ssid(escaped, network->ssid, network->ssid_len)

in the debug.

Quoted 10 lines · Joe Perches

This happens because escaped is declared the wrong size.

It'd be better to change

	char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
to
	DECLARE_SSID_BUF(escaped);
and use
	print_ssid(escaped, network->ssid, network->ssid_len)

in the debug.

if what you said is true:
it is better to delete escaped variable
use ssid instead of escaped, directly.

but I think the original author intended to use escaped instead of ssid

    DECLARE_SSID_BUF(ssid)  (line 5525, 5737)
    use ssid to print debug information directly
      (such as: line 5530..5535, 5545..5549, 5745..5749, ...)
    when need print additional information, use escaped
      (line 5559..5569, 5773..5782, 5791..5799)

so, I still suggest:
only fix the bug (use strlcpy instead of strncpy)
and not touch original features which orignal author intended using.

Regards

gchen.

in drivers/net/wireless/ipw2x00/ipw2200.c:

5519 static int ipw_find_adhoc_network(struct ipw_priv *priv,

 5520                                   struct ipw_network_match *match,
 5521                                   struct libipw_network *network,
 5522                                   int roaming)
 5523 {
 5524         struct ipw_supported_rates rates;
 5525         DECLARE_SSID_BUF(ssid);
 5526 
 5527         /* Verify that this network's capability is compatible with the
 5528          * current mode (AdHoc or Infrastructure) */
 5529         if ((priv->ieee->iw_mode == IW_MODE_ADHOC &&
 5530              !(network->capability & WLAN_CAPABILITY_IBSS))) {
 5531                 IPW_DEBUG_MERGE("Network '%s (%pM)' excluded due to "
 5532                                 "capability mismatch.\n",
 5533                                 print_ssid(ssid, network->ssid,
 5534                                            network->ssid_len),
 5535                                 network->bssid);
 5536                 return 0;
 5537         }
 5538 
 5539         if (unlikely(roaming)) {
 5540                 /* If we are roaming, then ensure check if this is a valid
 5541                  * network to try and roam to */
 5542                 if ((network->ssid_len != match->network->ssid_len) ||
 5543                     memcmp(network->ssid, match->network->ssid,
 5544                            network->ssid_len)) {
 5545                         IPW_DEBUG_MERGE("Network '%s (%pM)' excluded "
 5546                                         "because of non-network ESSID.\n",
 5547                                         print_ssid(ssid, network->ssid,
 5548                                                    network->ssid_len),
 5549                                         network->bssid);
 5550                         return 0;
 5551                 }
 5552         } else {
 5553                 /* If an ESSID has been configured then compare the broadcast
 5554                  * ESSID to ours */
 5555                 if ((priv->config & CFG_STATIC_ESSID) &&
 5556                     ((network->ssid_len != priv->essid_len) ||
 5557                      memcmp(network->ssid, priv->essid,
 5558                             min(network->ssid_len, priv->essid_len)))) {
 5559                         char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
 5560 
 5561                         strncpy(escaped,
 5562                                 print_ssid(ssid, network->ssid,
 5563                                            network->ssid_len),
 5564                                 sizeof(escaped));
 5565                         IPW_DEBUG_MERGE("Network '%s (%pM)' excluded "
 5566                                         "because of ESSID mismatch: '%s'.\n",
 5567                                         escaped, network->bssid,
 5568                                         print_ssid(ssid, priv->essid,

5569 priv->essid_len));
5570 return 0;
5571 }
5572 }
...

5732 static int ipw_best_network(struct ipw_priv *priv,
5733 struct ipw_network_match *match,
5734 struct libipw_network *network, int roaming)
5735 {
5736 struct ipw_supported_rates rates;
5737 DECLARE_SSID_BUF(ssid);
5738
5739 /* Verify that this network's capability is compatible with the
5740 * current mode (AdHoc or Infrastructure) */
5741 if ((priv->ieee->iw_mode == IW_MODE_INFRA &&
5742 !(network->capability & WLAN_CAPABILITY_ESS)) ||
5743 (priv->ieee->iw_mode == IW_MODE_ADHOC &&
5744 !(network->capability & WLAN_CAPABILITY_IBSS))) {
5745 IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded due to "
5746 "capability mismatch.\n",
5747 print_ssid(ssid, network->ssid,
5748 network->ssid_len),
5749 network->bssid);
5750 return 0;
5751 }
5752
5753 if (unlikely(roaming)) {
5754 /* If we are roaming, then ensure check if this is a valid
5755 * network to try and roam to */
5756 if ((network->ssid_len != match->network->ssid_len) ||
5757 memcmp(network->ssid, match->network->ssid,
5758 network->ssid_len)) {
5759 IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded "
5760 "because of non-network ESSID.\n",
5761 print_ssid(ssid, network->ssid,
5762 network->ssid_len),
5763 network->bssid);
5764 return 0;
5765 }
5766 } else {
5767 /* If an ESSID has been configured then compare the broadcast
5768 * ESSID to ours */
5769 if ((priv->config & CFG_STATIC_ESSID) &&
5770 ((network->ssid_len != priv->essid_len) ||
5771 memcmp(network->ssid, priv->essid,
5772 min(network->ssid_len, priv->essid_len)))) {
5773 char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
5774 strncpy(escaped,
5775 print_ssid(ssid, network->ssid,
5776 network->ssid_len),
5777 sizeof(escaped));
5778 IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded "

 5779                                         "because of ESSID mismatch: '%s'.\n",
 5780                                         escaped, network->bssid,
 5781                                         print_ssid(ssid, priv->essid,
 5782                                                    priv->essid_len));
 5783                         return 0;
 5784                 }
 5785         }
 5786 
 5787         /* If the old network rate is better than this one, don't bother
 5788          * testing everything else. */
 5789         if (match->network && match->network->stats.rssi > network->stats.rssi) {
 5790                 char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
 5791                 strncpy(escaped,
 5792                         print_ssid(ssid, network->ssid, network->ssid_len),
 5793                         sizeof(escaped));
 5794                 IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "
 5795                                 "'%s (%pM)' has a stronger signal.\n",
 5796                                 escaped, network->bssid,
 5797                                 print_ssid(ssid, match->network->ssid,

5798 match->network->ssid_len),
5799 match->network->bssid);
5800 return 0;
5801 }

but I think the original author intended to use escaped instead of ssid

    DECLARE_SSID_BUF(ssid)  (line 5525, 5737)
    use ssid to print debug information directly
      (such as: line 5530..5535, 5545..5549, 5745..5749, ...)
    when need print additional information, use escaped
      (line 5559..5569, 5773..5782, 5791..5799)

so, I still suggest:
only fix the bug (use strlcpy instead of strncpy)
and not touch original features which orignal author intended using.

Quoted 7 lines · Chen Gang

but I think the original author intended to use escaped instead of ssid

    DECLARE_SSID_BUF(ssid)  (line 5525, 5737)
    use ssid to print debug information directly
      (such as: line 5530..5535, 5545..5549, 5745..5749, ...)
    when need print additional information, use escaped
      (line 5559..5569, 5773..5782, 5791..5799)

Quoted 3 lines

so, I still suggest:
only fix the bug (use strlcpy instead of strncpy)
and not touch original features which orignal author intended using.

More likely John Linville just missed the conversions. 4+ years ago.

commit 9387b7caf3049168fc97a8a9111af8fe2143af18

Author: John W. Linville <[email protected]>
Date:   Tue Sep 30 20:59:05 2008 -0400

wireless: use individual buffers for printing ssid values

Also change escape_ssid to print_ssid to match print_mac semantics.

Signed-off-by John W. Linville <[email protected]>

Maybe these days this should be another vsprintf %p extension like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.

(or maybe extend %ph for ssids with %*phs, length, array)

But if and until then, I suggest this instead:

 drivers/net/wireless/ipw2x00/ipw2200.c | 38 ++++++++++++++--------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

drivers/net/wireless/ipw2x00/ipw2200.c +15 -24

index 844f201..3dc6a92 100644
@@ -5556,15 +5556,12 @@ static int ipw_find_adhoc_network(struct ipw_priv *priv,
 		    ((network->ssid_len != priv->essid_len) ||
 		     memcmp(network->ssid, priv->essid,
 			    min(network->ssid_len, priv->essid_len)))) {
-			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
+			DECLARE_SSID_BUF(escaped);
 
-			strncpy(escaped,
-				print_ssid(ssid, network->ssid,
-					   network->ssid_len),
-				sizeof(escaped));
-			IPW_DEBUG_MERGE("Network '%s (%pM)' excluded "
-					"because of ESSID mismatch: '%s'.\n",
-					escaped, network->bssid,
+			IPW_DEBUG_MERGE("Network '%s (%pM)' excluded because of ESSID mismatch: '%s'\n",
+					print_ssid(escaped, network->ssid,
+						   network->ssid_len),
+					network->bssid,
 					print_ssid(ssid, priv->essid,
 						   priv->essid_len));
 			return 0;

@@ -5770,14 +5767,11 @@ static int ipw_best_network(struct ipw_priv *priv,
 		    ((network->ssid_len != priv->essid_len) ||
 		     memcmp(network->ssid, priv->essid,
 			    min(network->ssid_len, priv->essid_len)))) {
-			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
-			strncpy(escaped,
-				print_ssid(ssid, network->ssid,
-					   network->ssid_len),
-				sizeof(escaped));
-			IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded "
-					"because of ESSID mismatch: '%s'.\n",
-					escaped, network->bssid,
+			DECLARE_SSID_BUF(escaped);
+			IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because of ESSID mismatch: '%s'\n",
+					print_ssid(escaped, network->ssid,
+						   network->ssid_len),
+					network->bssid,
 					print_ssid(ssid, priv->essid,
 						   priv->essid_len));
 			return 0;

@@ -5787,13 +5781,11 @@ static int ipw_best_network(struct ipw_priv *priv,
 	/* If the old network rate is better than this one, don't bother
 	 * testing everything else. */
 	if (match->network && match->network->stats.rssi > network->stats.rssi) {
-		char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
-		strncpy(escaped,
-			print_ssid(ssid, network->ssid, network->ssid_len),
-			sizeof(escaped));
-		IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "
-				"'%s (%pM)' has a stronger signal.\n",
-				escaped, network->bssid,
+		DECLARE_SSID_BUF(escaped);
+		IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because '%s (%pM)' has a stronger signal\n",
+				print_ssid(escaped, network->ssid,
+					   network->ssid_len),
+				network->bssid,
 				print_ssid(ssid, match->network->ssid,
 					   match->network->ssid_len),
 				match->network->bssid);
 
 
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Maybe these days this should be another vsprintf %p extension like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.

(or maybe extend %ph for ssids with %*phs, length, array)

Quoted 4 lines · Joe Perches

Maybe these days this should be another vsprintf %p extension like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.

(or maybe extend %ph for ssids with %*phs, length, array)

Maybe like this:

 lib/vsprintf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

lib/vsprintf.c +55 -2

index fab33a9..98916a0 100644
@@ -26,6 +26,7 @@
 #include <linux/math64.h>
 #include <linux/uaccess.h>
 #include <linux/ioport.h>
+#include <linux/ieee80211.h>
 #include <net/addrconf.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */

@@ -660,10 +661,59 @@ char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static noinline_for_stack
+char *ssid_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
+		  const char *fmt)
+{
+	int i, len = 1;		/* if we pass %*p, field width remains
+				   negative value, fallback to the default */
+
+	if (spec.field_width == 0)
+		/* nothing to print */
+		return buf;
+
+	if (ZERO_OR_NULL_PTR(addr))
+		/* NULL pointer */
+		return string(buf, end, NULL, spec);
+
+	if (spec.field_width > 0)
+		len = min_t(int, spec.field_width, IEEE80211_MAX_SSID_LEN);
+
+	for (i = 0; i < len && buf < end; i++) {
+		if (isprint(addr[i])) {
+			*buf++ = addr[i];
+			continue;
+		}
+		*buf++ = '\\';
+		if (buf >= end)
+			continue;
+		if (addr[i] == '\0')
+			*buf++ = '0';
+		else if (addr[i] == '\n')
+			*buf++ = 'n';
+		else if (addr[i] == '\r')
+			*buf++ = 'r';
+		else if (addr[i] == '\t')
+			*buf++ = 't';
+		else if (addr[i] == '\\')
+			*buf++ = '\\';
+		else {
+			if (buf < end)
+				*buf++ = ((addr[i] >> 6) & 7) + '0';
+			if (buf < end)
+				*buf++ = ((addr[i] >> 3) & 7) + '0';
+			if (buf < end)
+				*buf++ = ((addr[i] >> 0) & 7) + '0';
+		}
+	}
+
+	return buf;
+}
+
+static noinline_for_stack
 char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		 const char *fmt)
 {
-	int i, len = 1;		/* if we pass '%ph[CDN]', field witdh remains
+	int i, len = 1;		/* if we pass '%ph[CDN]', field width remains
 				   negative value, fallback to the default */
 	char separator;
 

@@ -695,7 +745,6 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 
 	for (i = 0; i < len && buf < end - 1; i++) {
 		buf = hex_byte_pack(buf, addr[i]);
-
 		if (buf < end && separator && i != len - 1)
 			*buf++ = separator;
 	}

@@ -1016,6 +1065,8 @@ int kptr_restrict __read_mostly;
  *             [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15]
  *           little endian output byte order is:
  *             [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15]
+ * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
+         characters with a leading \ and octal or [0nrt\]
  * - 'V' For a struct va_format which contains a format string * and va_list *,
  *       call vsnprintf(->format, *->va_list).
  *       Implements a "recursive vsnprintf".

@@ -1088,6 +1139,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		break;
 	case 'U':
 		return uuid_string(buf, end, ptr, spec, fmt);
+	case 'D':
+		return ssid_string(buf, end, ptr, spec, fmt);
 	case 'V':
 		{
 			va_list va;

Quoted 6 lines · Joe Perches

Quoted 4 lines · Joe Perches

Maybe these days this should be another vsprintf %p extension like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.

(or maybe extend %ph for ssids with %*phs, length, array)

excuse me:
I do not quite know how to reply the [RFC PATCH].
it would be better if you can tell me how to do for [RFC PATCH], thanks.

at least for me:
although it is good idea to add common, widely used features in public tools function.

You did fine except you unnecessarily quoted the entire original email. Remember to trim your replies please. _lots_ of people read these mailing lists and unnecessary quoting wastes all of their times.

Quoted 2 lines

Quoted 2 lines

at least for me:
although it is good idea to add common, widely used features in public tools function.

It's akin to most of the minor uuid/bluetooth mac, etc codes

Quoted 4 lines · Joe Perches

Maybe these days this should be another vsprintf %p extension like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.

(or maybe extend %ph for ssids with %*phs, length, array)

Maybe like this:

+ * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
+ characters with a leading \ and octal or [0nrt\]

Quoted 4 lines · Joe Perches

Quoted 3 lines · Joe Perches

Maybe these days this should be another vsprintf %p extension like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.

Quoted 6 lines

Quoted 1 line

(or maybe extend %ph for ssids with %*phs, length, array)

Maybe like this:

+ * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
+ characters with a leading \ and octal or [0nrt\]

Honestly, I'm not sure it's worth it.

print_ssid() is used in two or
three legacy drivers only, not in any modern driver, and is unlikely to be used in the more modern drivers due to tracing etc.

Quoted 3 lines · Johannes Berg

Quoted 3 lines

print_ssid() is used in two or
three legacy drivers only, not in any modern driver, and is unlikely to be used in the more modern drivers due to tracing etc.

Swell. It was just another way to correct those overrun errors Chen Gang found.

Quoted 7 lines · Joe Perches

Quoted 3 lines · Johannes Berg

Quoted 3 lines

print_ssid() is used in two or
three legacy drivers only, not in any modern driver, and is unlikely to be used in the more modern drivers due to tracing etc.

Swell. It was just another way to correct those overrun errors Chen Gang found.

sorry, I am not quite clear about what you said.

Quoted 1 line · Chen Gang

sorry, I am not quite clear about what you said.

You found some overrun errors and proposed a patch. Your solution could output incomplete SSIDs.

I proposed a different patch that would fully output any binary/non-ascii printable SSID.

I also proposed a different mechanism to avoid the overrun via printk and also avoid possibly excessive stack consumption as a means for John Linville to select 1 of the above options.

I don't care what is done with any of those proposed patches.

Clear?