diff mbox

[WT,6/6] mac80211: Tell user why beacons fail to parse.

Message ID 1372546738-25827-6-git-send-email-greearb@candelatech.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Ben Greear June 29, 2013, 10:58 p.m. UTC 
From: Ben Greear <greearb@candelatech.com>

Should help better debug dodgy APs and such.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/ieee80211_i.h |    2 +
 net/mac80211/mlme.c        |    4 +-
 net/mac80211/scan.c        |    6 +++
 net/mac80211/util.c        |   79 +++++++++++++++++++++++++++++++++++++++----
 4 files changed, 81 insertions(+), 10 deletions(-)

Comments

Johannes Berg July 11, 2013, 8:59 a.m. UTC | #1 
On Sat, 2013-06-29 at 15:58 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> Should help better debug dodgy APs and such.

This isn't a bad idea, but I think instead of storing the message:

> @@ -110,6 +110,7 @@ struct ieee80211_bss {
>  
>  	/* Keep track of what bits of information we have valid info for. */
>  	u8 valid_data;
> +	char corrupt_elems_msg[80];

you should store a "what's bad" type field and the broken IE number or
so, to reduce memory usage

> @@ -4341,8 +4341,8 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
>  				corrupt_type = "beacon";
>  		} else if (bss->corrupt_data & IEEE80211_BSS_CORRUPT_PROBE_RESP)
>  			corrupt_type = "probe response";
> -		sdata_info(sdata, "associating with AP with corrupt %s\n",
> -			   corrupt_type);
> +		sdata_info(sdata, "associating with AP with corrupt %s, reason: %s\n",
> +			   corrupt_type, bss->corrupt_elems_msg);

and only actually format the message here. That also reduces overhead
during beacon/probe response processing.
 
> +				snprintf(elems->parse_err_msg,
> +					 sizeof(elems->parse_err_msg),
> +					 "seen id: %i already", id);

Your snprintf() usage is also unsafe.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Greear July 11, 2013, 3:10 p.m. UTC | #2 
On 07/11/2013 01:59 AM, Johannes Berg wrote:
> On Sat, 2013-06-29 at 15:58 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Should help better debug dodgy APs and such.
>
> This isn't a bad idea, but I think instead of storing the message:
>
>> @@ -110,6 +110,7 @@ struct ieee80211_bss {
>>
>>   	/* Keep track of what bits of information we have valid info for. */
>>   	u8 valid_data;
>> +	char corrupt_elems_msg[80];
>
> you should store a "what's bad" type field and the broken IE number or
> so, to reduce memory usage

I thought of this, but the problem is then you cannot tell the details
(for instance the actual lengths when length is bad, the ID that is duplicated, etc).
I figure if we are going to provide the info to the user, we might as well
be specific about it.

>> +				snprintf(elems->parse_err_msg,
>> +					 sizeof(elems->parse_err_msg),
>> +					 "seen id: %i already", id);
>
> Your snprintf() usage is also unsafe.

What is unsafe about it?

Thanks,
Ben
Johannes Berg July 11, 2013, 3:17 p.m. UTC | #3 
On Thu, 2013-07-11 at 08:10 -0700, Ben Greear wrote:

> >>   	/* Keep track of what bits of information we have valid info for. */
> >>   	u8 valid_data;
> >> +	char corrupt_elems_msg[80];
> >
> > you should store a "what's bad" type field and the broken IE number or
> > so, to reduce memory usage
> 
> I thought of this, but the problem is then you cannot tell the details
> (for instance the actual lengths when length is bad, the ID that is duplicated, etc).
> I figure if we are going to provide the info to the user, we might as well
> be specific about it.

It doesn't seem so difficult?

enum ies_parse_error {
	NO_ERROR,
	BAD_IE_LEN, // uses eid+len
	DUPLICATE_IE, // uses eid
	...
} parse_error;
int parse_error_eid, parse_error_len;

> >> +				snprintf(elems->parse_err_msg,
> >> +					 sizeof(elems->parse_err_msg),
> >> +					 "seen id: %i already", id);
> >
> > Your snprintf() usage is also unsafe.
> 
> What is unsafe about it?

using printk("%s", result) is unsafe due to potentially missing
NUL-termination.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index f36f120..809c9a5 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -110,6 +110,7 @@  struct ieee80211_bss {
 
 	/* Keep track of what bits of information we have valid info for. */
 	u8 valid_data;
+	char corrupt_elems_msg[80];
 };
 
 /**
@@ -1258,6 +1259,7 @@  struct ieee802_11_elems {
 
 	/* whether a parse error occurred while retrieving these elements */
 	bool parse_error;
+	char parse_err_msg[80];
 };
 
 static inline struct ieee80211_local *hw_to_local(
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index ae31968..42edd6c 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4341,8 +4341,8 @@  int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
 				corrupt_type = "beacon";
 		} else if (bss->corrupt_data & IEEE80211_BSS_CORRUPT_PROBE_RESP)
 			corrupt_type = "probe response";
-		sdata_info(sdata, "associating with AP with corrupt %s\n",
-			   corrupt_type);
+		sdata_info(sdata, "associating with AP with corrupt %s, reason: %s\n",
+			   corrupt_type, bss->corrupt_elems_msg);
 	}
 
 	return 0;
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 1b122a7..9dc9c98 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -86,6 +86,8 @@  ieee80211_bss_info_update(struct ieee80211_local *local,
 		bss->device_ts_presp = rx_status->device_timestamp;
 
 	if (elems->parse_error) {
+		strncpy(bss->corrupt_elems_msg, elems->parse_err_msg,
+			sizeof(bss->corrupt_elems_msg));
 		if (beacon)
 			bss->corrupt_data |= IEEE80211_BSS_CORRUPT_BEACON;
 		else
@@ -95,6 +97,10 @@  ieee80211_bss_info_update(struct ieee80211_local *local,
 			bss->corrupt_data &= ~IEEE80211_BSS_CORRUPT_BEACON;
 		else
 			bss->corrupt_data &= ~IEEE80211_BSS_CORRUPT_PROBE_RESP;
+		if (!(bss->corrupt_data &
+		      (IEEE80211_BSS_CORRUPT_BEACON |
+		       IEEE80211_BSS_CORRUPT_PROBE_RESP)))
+			bss->corrupt_elems_msg[0] = 0;
 	}
 
 	/* save the ERP value so that it is available at association time */
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 2265445..7e6a4f3 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -692,6 +692,10 @@  u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
 
 		if (elen > left) {
 			elems->parse_error = true;
+			snprintf(elems->parse_err_msg,
+				 sizeof(elems->parse_err_msg),
+				 "elen: %hhu > left: %zu",
+				 elen, left);
 			break;
 		}
 
@@ -731,6 +735,9 @@  u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
 		 */
 			if (test_bit(id, seen_elems)) {
 				elems->parse_error = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "seen id: %i already", id);
 				left -= elen;
 				pos += elen;
 				continue;
@@ -762,8 +769,14 @@  u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
 			if (elen >= sizeof(struct ieee80211_tim_ie)) {
 				elems->tim = (void *)pos;
 				elems->tim_len = elen;
-			} else
+			} else {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "EID_TIM size wrong, elen: %hhu  sizeof(tim_ie): %zu",
+					 elen,
+					 sizeof(struct ieee80211_tim_ie));
+			}
 			break;
 		case WLAN_EID_CHALLENGE:
 			elems->challenge = pos;
@@ -806,32 +819,61 @@  u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
 		case WLAN_EID_HT_CAPABILITY:
 			if (elen >= sizeof(struct ieee80211_ht_cap))
 				elems->ht_cap_elem = (void *)pos;
-			else
+			else {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "HT_CAPAB size wrong, elen: %hhu  sizeof(ht_cap): %zu",
+					 elen,
+					 sizeof(struct ieee80211_ht_cap));
+			}
 			break;
 		case WLAN_EID_HT_OPERATION:
 			if (elen >= sizeof(struct ieee80211_ht_operation))
 				elems->ht_operation = (void *)pos;
-			else
+			else {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "HT_OPER size wrong, elen: %hhu  sizeof(ht_oper): %zu",
+					 elen,
+					 sizeof(struct ieee80211_ht_operation));
+			}
 			break;
 		case WLAN_EID_VHT_CAPABILITY:
 			if (elen >= sizeof(struct ieee80211_vht_cap))
 				elems->vht_cap_elem = (void *)pos;
-			else
+			else {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "EID_VHT size wrong, elen: %hhu  sizeof(vht_cap): %zu",
+					 elen,
+					 sizeof(struct ieee80211_vht_cap));
+			}
 			break;
 		case WLAN_EID_VHT_OPERATION:
 			if (elen >= sizeof(struct ieee80211_vht_operation))
 				elems->vht_operation = (void *)pos;
-			else
+			else {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "VHT_OPER size wrong, elen: %hhu  sizeof(vht_oper): %zu",
+					 elen,
+					 sizeof(struct ieee80211_vht_operation));
+			}
 			break;
 		case WLAN_EID_OPMODE_NOTIF:
 			if (elen > 0)
 				elems->opmode_notif = pos;
-			else
+			else {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "OPMODE_NOTIF has elen > 0: %hhu",
+					 elen);
+			}
 			break;
 		case WLAN_EID_MESH_ID:
 			elems->mesh_id = pos;
@@ -840,8 +882,14 @@  u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
 		case WLAN_EID_MESH_CONFIG:
 			if (elen >= sizeof(struct ieee80211_meshconf_ie))
 				elems->mesh_config = (void *)pos;
-			else
+			else {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "MESH_CONFIG size wrong, elen: %hhu  sizeof(meshconf_ie): %zu",
+					 elen,
+					 sizeof(struct ieee80211_meshconf_ie));
+			}
 			break;
 		case WLAN_EID_PEER_MGMT:
 			elems->peering = pos;
@@ -866,12 +914,23 @@  u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
 		case WLAN_EID_RANN:
 			if (elen >= sizeof(struct ieee80211_rann_ie))
 				elems->rann = (void *)pos;
-			else
+			else {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "EID_RANN size wrong, elen: %hhu  sizeof(rann_ie): %zu",
+					 elen,
+					 sizeof(struct ieee80211_rann_ie));
+			}
 			break;
 		case WLAN_EID_CHANNEL_SWITCH:
 			if (elen != sizeof(struct ieee80211_channel_sw_ie)) {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "CH_SWITCH size wrong, elen: %hhu  sizeof(sw_ie): %zu",
+					 elen,
+					 sizeof(struct ieee80211_channel_sw_ie));
 				break;
 			}
 			elems->ch_switch_ie = (void *)pos;
@@ -925,6 +984,10 @@  u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
 		case WLAN_EID_PWR_CONSTRAINT:
 			if (elen != 1) {
 				elem_parse_failed = true;
+				snprintf(elems->parse_err_msg,
+					 sizeof(elems->parse_err_msg),
+					 "PWR_CONSTRAINT size not 1, elen: %hhu",
+					 elen);
 				break;
 			}
 			elems->pwr_constr_elem = pos;