The fields must be null-terminated, or IPW_DEBUG_ASSOC will cause issue.
Signed-off-by: Chen Gang <[email protected]>
---
drivers/net/wireless/ipw2x00/ipw2200.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
index 844f201..c85261b 100644
--- a/drivers/net/wireless/ipw2x00/ipw2200.c
+++ b/drivers/net/wireless/ipw2x00/ipw2200.c
@@ -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));
@@ -5788,7 +5788,7 @@ static int ipw_best_network(struct ipw_priv *priv,
* testing everything else. */
if (match->network && match->network->stats.rssi > network->stats.rssi) {
char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
- strncpy(escaped,
+ strlcpy(escaped,
print_ssid(ssid, network->ssid, network->ssid_len),
sizeof(escaped));
IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "
--
1.7.10.4
On Sat, 2013-01-05 at 21:41 +0800, Chen Gang wrote:
> The fields must be null-terminated, or IPW_DEBUG_ASSOC will cause issue.
[]
> diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
[]
> @@ -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));
> @@ -5788,7 +5788,7 @@ static int ipw_best_network(struct ipw_priv *priv,
> * testing everything else. */
> if (match->network && match->network->stats.rssi > network->stats.rssi) {
> char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
> - strncpy(escaped,
> + strlcpy(escaped,
> print_ssid(ssid, network->ssid, network->ssid_len),
> sizeof(escaped));
> IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "
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.
于 2013年01月07日 11:19, Joe Perches 写道:
> On Mon, 2013-01-07 at 10:49 +0800, Chen Gang wrote:
>> 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.
>
> More likely John Linville just missed the conversions.
> 4+ years ago.
>
I wonder why it is not integrated into main branch.
maybe we miss it.
if so, I suggest to integrate it (better also add Reported-by [email protected] :-) )
maybe original author intended using short length.
if so, I suggest to integrate my patch (using strlcpy instead of strncpy).
for me:
using long size instead of original short size, will change the output format.
it seems not a good idea to change the original output format.
(especially, the original output format has existence 4+ years).
so, at least:
for only fixing bug, not touching original features
it is an executable method (is a valuable patch).
although maybe it is not a best method (is not a very good patch).
Regards
gchen.
> 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(-)
>
> diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
> index 844f201..3dc6a92 100644
> --- a/drivers/net/wireless/ipw2x00/ipw2200.c
> +++ b/drivers/net/wireless/ipw2x00/ipw2200.c
> @@ -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
>
--
Chen Gang
Flying Transformer
于 2013年01月07日 10:49, Chen Gang 写道:
> 于 2013年01月05日 22:42, 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.
>
oh, sorry, it is my fault.
we need use duplicate buffer to print different contents, at the same time.
:-)
but I still suggest to keep original author using
maybe he intend to keep the print size for output format
so I think it is better to only fix bug, not touch the features.
Regards
gchen.
> 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 }
>
--
Chen Gang
Flying Transformer
On Tue, 2013-01-08 at 10:57 +0800, Chen Gang wrote:
> 于 2013年01月08日 01:22, Joe Perches 写道:
> > On Mon, 2013-01-07 at 08:47 +0100, Johannes Berg wrote:
> >> > 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.
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?
On Mon, 2013-01-07 at 14:07 +0800, Chen Gang wrote:
> 于 2013年01月07日 12:49, Joe Perches 写道:
> > On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> >> 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.
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.
> 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
in vsprintf I've added.
cheers, Joe
于 2013年01月07日 12:49, Joe Perches 写道:
> On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
>> 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.
after searching the relative source code:
it seems print ssid is not quite widely used (although it is a common feature).
it is used by drivers/net/wireless/libertas
it is used by drivers/net/wireless/ipw2x00
no additional using in current kernel source code wide.
if another modules want to print ssid,
they can still call print_ssid now (EXPORT_SYMBOL in net/wireless).
so at least now, it is not necessary to add this feature to public tools function.
Regards
gchen.
> Maybe like this:
>
> lib/vsprintf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index fab33a9..98916a0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -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;
>
>
>
>
--
Chen Gang
Asianux Corporation
于 2013年01月07日 14:37, Joe Perches 写道:
> 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.
>
ok, thank you.
I should notice, next time. :-)
>> > 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
after see the comments for function pointer in lib/vsprintf.c
sorry for I do not think it is suitable to add ssid in vsprintf
all formate features in vsprintf are face to kernel wide, not for one sub system
one exception is for mac and ip:
they have already been well known in kernel wide
every one knows about mac and ip.
I feel, our ssid is not quite well known in kernel wide
maybe current place (net/wireless/) is not quite suitable for print_ssid:
for bluetooth, it is not belong to net/wireless.
if necessary, I suggest to reconstruct it within net sub system wide, not in kernel wide.
(it is also my fault for my original reply, it is not quite clear)
Regards
--
Chen Gang
Asianux Corporation
On Mon, 2013-01-07 at 10:49 +0800, Chen Gang wrote:
> 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.
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(-)
diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
index 844f201..3dc6a92 100644
--- a/drivers/net/wireless/ipw2x00/ipw2200.c
+++ b/drivers/net/wireless/ipw2x00/ipw2200.c
@@ -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);
于 2013年01月05日 22:42, 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 }
--
Chen Gang
Asianux Corporation
于 2013年01月08日 01:22, Joe Perches 写道:
> On Mon, 2013-01-07 at 08:47 +0100, Johannes Berg wrote:
>> > 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.
--
Chen Gang
Asianux Corporation
于 2013年01月08日 11:11, Joe Perches 写道:
> On Tue, 2013-01-08 at 10:57 +0800, Chen Gang wrote:
>> 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?
>
ok
--
Chen Gang
Asianux Corporation
On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> 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(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index fab33a9..98916a0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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;
On Mon, 2013-01-07 at 08:47 +0100, Johannes Berg wrote:
> On Sun, 2013-01-06 at 20:49 -0800, Joe Perches wrote:
> > On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> > > 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\]
>
> Honestly, I'm not sure it's worth it.
Neither am I really, the only value I see in it is the
reduction in stack consumption by 100 bytes or so per use.
> 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.
On Sun, 2013-01-06 at 20:49 -0800, Joe Perches wrote:
> On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> > 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\]
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.
johannes