[PATCH 0/4] cfg80211: few country IE parse fixes

[PATCH 0/4] cfg80211: few country IE parse fixes

[Luis R. Rodriguez]

John, this series has a few country IE fixes some of which
Benoit reported. The fix I made goes in with an enhancement
which reduces the number of rules created if the AP is stupid
and sends congigious channels, never making use of the number
of channels in the triplets.

I've tested this against an AP issuing these country IE
triplets below:

[1]
[2]
[36]
[40]

[1]
[2-4]
[5-12]
[36]
[40-44]

[1-4]
[5-7]
[36-44]
[48-64]

[36-36]
[40-40]
[44-44]
[48-48]
[52-52]
[56-56]
[60-60]
[64-64]
[100-100]
[104-104]
[108-108]
[112-112]
[116-116]
[120-120]
[124-124]
[128-128]
[132-132]
[136-136]
[140-140]

All these country IEs now only produce two triplets on regulatory rules.

I generated these with hostapd using a debug patch for country IEs:

http://bombadil.infradead.org/~mcgrof/patches/hostapd/enable-11d-debugging.patch

Luis R. Rodriguez (4):
  cfg80211: add debug print when we drop a bogus country IE
  cfg80211: fix 2 GHz subband calculation for country IEs
  cfg80211: process the max power on a country IE
  cfg80211: Fix country IE parsing for single channel triplets

 net/wireless/reg.c |  235 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 227 insertions(+), 8 deletions(-)

[PATCH 1/4] cfg80211: add debug print when we drop a bogus country IE

[Luis R. Rodriguez]

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index f79d661..389247c 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1806,8 +1806,10 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 		goto out;
 
 	rd = country_ie_2_rd(country_ie, country_ie_len, &checksum);
-	if (!rd)
+	if (!rd) {
+		REG_DBG_PRINT("cfg80211: Ignoring bogus country IE\n");
 		goto out;
+	}
 
 	/*
 	 * This will not happen right now but we leave it here for the
-- 
1.6.3.3

[PATCH 2/4] cfg80211: fix 2 GHz subband calculation for country IEs

[Luis R. Rodriguez]

Country IEs triplets are getting an extra channel with
the current count. This does not affect regulatory because
we always took the intersection between what the AP gave
and what CRDA believed is correct.

This however does fix processing some Country IEs with multiple
sequential 2 GHz triplets. Since our parser and the spec mandates
all channels be monitonically increasing we would drop the IE after
noticing the second triplet begins on a channel already processed.
APs that send these type of country IEs seems rare though.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 389247c..7223a7a 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -555,7 +555,7 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 		/* 2 GHz */
 		if (triplet->chans.first_channel <= 14)
 			end_channel = triplet->chans.first_channel +
-				triplet->chans.num_channels;
+				triplet->chans.num_channels - 1;
 		else
 			/*
 			 * 5 GHz -- For example in country IEs if the first
@@ -655,7 +655,7 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 		/* 2 GHz */
 		if (triplet->chans.first_channel <= 14)
 			end_channel = triplet->chans.first_channel +
-				triplet->chans.num_channels;
+				triplet->chans.num_channels -1;
 		else
 			end_channel =  triplet->chans.first_channel +
 				(4 * (triplet->chans.num_channels - 1));
-- 
1.6.3.3

Re: [PATCH 2/4] cfg80211: fix 2 GHz subband calculation for country IEs

[Jouni Malinen]

On Thu, Jan 07, 2010 at 05:24:55PM -0500, Luis R. Rodriguez wrote:
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -555,7 +555,7 @@ static struct ieee80211_regdomain *country_ie_2_rd(
>  		/* 2 GHz */
>  		if (triplet->chans.first_channel <= 14)

By the way, that looks completely bogus.. Channel number cannot be used
to figure out whether this is 2.4 GHz or 5 GHz band (e.g., channel 8 is
used in both). This really should be based on the band the frame with
the Country IE was received.

>  			end_channel = triplet->chans.first_channel +
> -				triplet->chans.num_channels;
> +				triplet->chans.num_channels - 1;

Could be reasonable to verify that num_channels is not 0.. Based on a
quick look, I did not notice places where negative end_channel value
would trigger bad problems, but it would be safer to validate that the
received information is valid before using it.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 2/4] cfg80211: fix 2 GHz subband calculation for country IEs

[Luis R. Rodriguez]

On Fri, Jan 8, 2010 at 5:14 AM, Jouni Malinen <j@w1.fi> wrote:
> On Thu, Jan 07, 2010 at 05:24:55PM -0500, Luis R. Rodriguez wrote:
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -555,7 +555,7 @@ static struct ieee80211_regdomain *country_ie_2_rd(
>>               /* 2 GHz */
>>               if (triplet->chans.first_channel <= 14)
>
> By the way, that looks completely bogus.. Channel number cannot be used
> to figure out whether this is 2.4 GHz or 5 GHz band (e.g., channel 8 is
> used in both). This really should be based on the band the frame with
> the Country IE was received.

This seems to me more like an issue with the spec. Doing it based on
the received band is obviously possible but what if the AP wants to
send out Country IE information for both channel 8 on 2 GHz and
channel 8 on 5 GHz?

>>                       end_channel = triplet->chans.first_channel +
>> -                             triplet->chans.num_channels;
>> +                             triplet->chans.num_channels - 1;
>
> Could be reasonable to verify that num_channels is not 0.. Based on a
> quick look, I did not notice places where negative end_channel value
> would trigger bad problems, but it would be safer to validate that the
> received information is valid before using it.

Good point, thanks, will send another patch to deal with that case.

  Luis

Re: [PATCH 2/4] cfg80211: fix 2 GHz subband calculation for country IEs

[Jouni Malinen]

On Fri, Jan 08, 2010 at 01:41:58PM -0800, Luis R. Rodriguez wrote:
> On Fri, Jan 8, 2010 at 5:14 AM, Jouni Malinen <j@w1.fi> wrote:
> > By the way, that looks completely bogus.. Channel number cannot be used
> > to figure out whether this is 2.4 GHz or 5 GHz band (e.g., channel 8 is
> > used in both). This really should be based on the band the frame with
> > the Country IE was received.
> 
> This seems to me more like an issue with the spec. Doing it based on
> the received band is obviously possible but what if the AP wants to
> send out Country IE information for both channel 8 on 2 GHz and
> channel 8 on 5 GHz?
> 

I don't think the original Country IE design from IEEE Std 802.11d-2001
really supported such use, i.e., you can only describe the channels from
your own band with that design. Have you really seen APs advertising
channels from the other band in their Country IE?

I would assume you could now use the Regulatory Extension mechanism to
define another regulatory class and subband followed by channel triplets
to do that. However, even in that case, the first triplets in the IE
really needs to be for the band the frame is send on. Once the
Regulatory Extension Identifier is used, further triplets should be
interpreted differently. Anyway, I have no idea whether anyone really
supports this extension (or can parse the standard to figure out how it
would be used ;-)..

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 2/4] cfg80211: fix 2 GHz subband calculation for country IEs

[Luis R. Rodriguez]

On Fri, Jan 8, 2010 at 3:20 PM, Jouni Malinen <j@w1.fi> wrote:
> On Fri, Jan 08, 2010 at 01:41:58PM -0800, Luis R. Rodriguez wrote:
>> On Fri, Jan 8, 2010 at 5:14 AM, Jouni Malinen <j@w1.fi> wrote:
>> > By the way, that looks completely bogus.. Channel number cannot be used
>> > to figure out whether this is 2.4 GHz or 5 GHz band (e.g., channel 8 is
>> > used in both). This really should be based on the band the frame with
>> > the Country IE was received.
>>
>> This seems to me more like an issue with the spec. Doing it based on
>> the received band is obviously possible but what if the AP wants to
>> send out Country IE information for both channel 8 on 2 GHz and
>> channel 8 on 5 GHz?
>>
>
> I don't think the original Country IE design from IEEE Std 802.11d-2001
> really supported such use, i.e., you can only describe the channels from
> your own band with that design. Have you really seen APs advertising
> channels from the other band in their Country IE?
>
> I would assume you could now use the Regulatory Extension mechanism to
> define another regulatory class and subband followed by channel triplets
> to do that. However, even in that case, the first triplets in the IE
> really needs to be for the band the frame is send on. Once the
> Regulatory Extension Identifier is used, further triplets should be
> interpreted differently. Anyway, I have no idea whether anyone really
> supports this extension (or can parse the standard to figure out how it
> would be used ;-)..

Heh alright, I'll change this to match the rx'd band first but that
change is a bit big for stable so we'll have to deal with how we have
it now for stable. I'll submit the changes for wireless-testing. Now
last question -- although I haven't seen APs send country IEs for two
bands in case we do, how do you want to treat those channels?

Does hostapd send just one band or two bands on the country IE?

  Luis

Re: [PATCH 2/4] cfg80211: fix 2 GHz subband calculation for country IEs

[Jouni Malinen]

On Fri, Jan 08, 2010 at 04:28:17PM -0800, Luis R. Rodriguez wrote:

> Heh alright, I'll change this to match the rx'd band first but that
> change is a bit big for stable so we'll have to deal with how we have
> it now for stable. I'll submit the changes for wireless-testing. Now
> last question -- although I haven't seen APs send country IEs for two
> bands in case we do, how do you want to treat those channels?

What exactly do you mean with sending two bands? By using the Regulatory
Extension Identifier? I didn't think we support it.. In general, I would
not bother doing anything special until someone can show that such an AP
exist.

> Does hostapd send just one band or two bands on the country IE?

Only one; it does not support Regulatory Extension Identifier and I
don't really see any point in advertising more than the current band.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 2/4] cfg80211: fix 2 GHz subband calculation for country IEs

[Benoit PAPILLAULT]

Jouni Malinen a écrit :
> On Fri, Jan 08, 2010 at 04:28:17PM -0800, Luis R. Rodriguez wrote:
>
>   
>> Heh alright, I'll change this to match the rx'd band first but that
>> change is a bit big for stable so we'll have to deal with how we have
>> it now for stable. I'll submit the changes for wireless-testing. Now
>> last question -- although I haven't seen APs send country IEs for two
>> bands in case we do, how do you want to treat those channels?
>>     
>
> What exactly do you mean with sending two bands? By using the Regulatory
> Extension Identifier? I didn't think we support it.. In general, I would
> not bother doing anything special until someone can show that such an AP
> exist.
>
>   
>> Does hostapd send just one band or two bands on the country IE?
>>     
>
> Only one; it does not support Regulatory Extension Identifier and I
> don't really see any point in advertising more than the current band.
>
>   
I was going to say the same as Jouni : channel number cannot be used to 
determine the frequency band (2.4GHz versus 5GHz). Channel 1 exists both 
in 2.4GHz and 5GHz. For 5GHz, I think the 802.11 specs mentions a 
"Channel starting frequency is defined as dot11ChannelStartingFactor × 
500 kHz or is defined as 5 GHz for systems where 
dot11RegulatoryClassesRequired is false or not defined." (17.3.8.3.2). 
This is based on the RegulatoryClass as defined in Annex J which is 
transmitted in Country IE as Regulatory Extension Identifier.

There are AP that are operating on both bands (either single radio or 
dual radio). We could imagine a scenario where the AP sends a Channel 
Switch Announcement IE asking to switch from a 2.4GHz channel to a 5GHz 
channel. At this point, the STA should be aware of regulatory 
restrictions that may exist in the 5GHz band. In this case, the STA must 
wait for a beacon to show up in the 5GHz band (see 11.1.3.3 Initializing 
a BSS) and we are on the safe side by assuming that it applies on the 
same band.

Regards,
Benoit

[PATCH 3/4] cfg80211: process the max power on a country IE

[Luis R. Rodriguez]

The max power from each country IE triplet was being ignored.
This fix isn't critical as CRDA was always being used for the lower
limit, but we should process it in case the AP still wants to
decrease power output even more for whatever reason.

Reported-by: Benoit PAPILLAULT <benoit.papillault@free.fr>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 7223a7a..1695d62 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -680,7 +680,7 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 		 */
 		freq_range->max_bandwidth_khz = MHZ_TO_KHZ(40);
 		power_rule->max_antenna_gain = DBI_TO_MBI(100);
-		power_rule->max_eirp = DBM_TO_MBM(100);
+		power_rule->max_eirp = DBM_TO_MBM(triplet->chans.max_power);
 
 		country_ie += 3;
 		country_ie_len -= 3;
-- 
1.6.3.3

[PATCH 4/4] cfg80211: Fix country IE parsing for single channel triplets

[Luis R. Rodriguez]

This enhances the way we parse country IEs to minimize
the number of regulatory rules that we create. It also fixes
our current implementation which treated country IE triplets
with only one channel as one independed regulatory rule even
though adjecent rules were also being provided.

Without this patch APs which send country IE information with
a channel triplet for each individual channel will force cfg80211
to deny HT40 operation as a regulatory rule would have been created
independently for each channel and as such configured only for
20 MHz operation.

Although 802.11n APs which send country IEs triplets in this fassion
are likely rare Benoit reports this against the Ubiquity NanoStation M5,
with Country "FR" and HT40 enabled.

Since we now have a helper which parses the triplets in intermediate
steps we now take care extra care to process padding.

Reported-by: Benoit PAPILLAULT <benoit.papillault@free.fr>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |  225 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 221 insertions(+), 4 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 1695d62..f0859ea 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -485,6 +485,178 @@ static bool freq_in_rule_band(const struct ieee80211_freq_range *freq_range,
 }
 
 /*
+ * Some APs may send a country IE triplet for each channel they
+ * support and while this is completely overkill and silly we still
+ * need to support it. We avoid making a single rule for each channel
+ * though and to help us with this we use this helper to find the
+ * actual subband end channel. These type of country IE triplet
+ * scenerios are handled then, all yielding two regulaotry rules from
+ * parsing a country IE:
+ *
+ * [1]
+ * [2]
+ * [36]
+ * [40]
+ *
+ * [1]
+ * [2-4]
+ * [5-12]
+ * [36]
+ * [40-44]
+ *
+ * [1-4]
+ * [5-7]
+ * [36-44]
+ * [48-64]
+ *
+ * [36-36]
+ * [40-40]
+ * [44-44]
+ * [48-48]
+ * [52-52]
+ * [56-56]
+ * [60-60]
+ * [64-64]
+ * [100-100]
+ * [104-104]
+ * [108-108]
+ * [112-112]
+ * [116-116]
+ * [120-120]
+ * [124-124]
+ * [128-128]
+ * [132-132]
+ * [136-136]
+ * [140-140]
+ *
+ * Returns 0 if the IE has been found to be invalid in the middle
+ * somewhere.
+ */
+static int max_subband_chan(int orig_cur_chan,
+			    int orig_end_channel,
+			    s8 orig_max_power,
+			    u8 **country_ie,
+			    u8 *country_ie_len)
+{
+	u8 *triplets_start = *country_ie;
+	u8 len_at_triplet = *country_ie_len;
+	int end_subband_chan = orig_end_channel;
+	enum ieee80211_band band;
+
+	/*
+	 * We'll deal with padding for the caller unless
+	 * its not immediate and we don't process any channels
+	 */
+	if (*country_ie_len == 1) {
+		*country_ie += 1;
+		*country_ie_len -= 1;
+		return orig_end_channel;
+	}
+
+	/* Move to the next triplet and then start search */
+	*country_ie += 3;
+	*country_ie_len -= 3;
+
+	if (orig_cur_chan <= 14)
+		band = IEEE80211_BAND_2GHZ;
+	else
+		band = IEEE80211_BAND_5GHZ;
+
+	while (*country_ie_len >= 3) {
+		int end_channel = 0;
+		struct ieee80211_country_ie_triplet *triplet =
+			(struct ieee80211_country_ie_triplet *) *country_ie;
+		int cur_channel = 0, next_expected_chan;
+		enum ieee80211_band next_band = IEEE80211_BAND_2GHZ;
+
+		/* means last triplet is completely unrelated to this one */
+		if (triplet->ext.reg_extension_id >=
+				IEEE80211_COUNTRY_EXTENSION_ID) {
+			*country_ie -= 3;
+			*country_ie_len += 3;
+			break;
+		}
+
+		if (triplet->chans.first_channel == 0) {
+			*country_ie += 1;
+			*country_ie_len -= 1;
+			if (*country_ie_len != 0)
+				return 0;
+			break;
+		}
+
+		/* Monitonically increasing channel order */
+		if (triplet->chans.first_channel <= end_subband_chan)
+			return 0;
+
+		/* 2 GHz */
+		if (triplet->chans.first_channel <= 14) {
+			end_channel = triplet->chans.first_channel +
+				triplet->chans.num_channels - 1;
+		}
+		else {
+			end_channel =  triplet->chans.first_channel +
+				(4 * (triplet->chans.num_channels - 1));
+			next_band = IEEE80211_BAND_5GHZ;
+		}
+
+		if (band != next_band) {
+			*country_ie -= 3;
+			*country_ie_len += 3;
+			break;
+		}
+
+		if (orig_max_power != triplet->chans.max_power) {
+			*country_ie -= 3;
+			*country_ie_len += 3;
+			break;
+		}
+
+		cur_channel = triplet->chans.first_channel;
+
+		/* The key is finding the right next expected channel */
+		if (band == IEEE80211_BAND_2GHZ)
+			next_expected_chan = end_subband_chan + 1;
+		 else
+			next_expected_chan = end_subband_chan + 4;
+
+		if (cur_channel != next_expected_chan) {
+			*country_ie -= 3;
+			*country_ie_len += 3;
+			break;
+		}
+
+		end_subband_chan = end_channel;
+
+		/* Move to the next one */
+		*country_ie += 3;
+		*country_ie_len -= 3;
+
+		/*
+		 * Padding needs to be dealt with if we processed
+		 * some channels.
+		 */
+		if (*country_ie_len == 1) {
+			*country_ie += 1;
+			*country_ie_len -= 1;
+			break;
+		}
+
+		/* If seen, the IE is invalid */
+		if (*country_ie_len == 2)
+			return 0;
+	}
+
+	if (end_subband_chan == orig_end_channel) {
+		*country_ie = triplets_start;
+		*country_ie_len = len_at_triplet;
+		return orig_end_channel;
+	}
+
+	return end_subband_chan;
+}
+
+/*
  * Converts a country IE to a regulatory domain. A regulatory domain
  * structure has a lot of information which the IE doesn't yet have,
  * so for the other values we use upper max values as we will intersect
@@ -552,6 +724,19 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 			continue;
 		}
 
+		/*
+		 * APs can add padding to make length divisible
+		 * by two, required by the spec.
+		 */
+		if (triplet->chans.first_channel == 0) {
+			country_ie++;
+			country_ie_len--;
+			/* This is expected to be at the very end only */
+			if (country_ie_len != 0)
+				return NULL;
+			break;
+		}
+
 		/* 2 GHz */
 		if (triplet->chans.first_channel <= 14)
 			end_channel = triplet->chans.first_channel +
@@ -570,6 +755,20 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 				(4 * (triplet->chans.num_channels - 1));
 
 		cur_channel = triplet->chans.first_channel;
+
+		/*
+		 * Enhancement for APs that send a triplet for every channel
+		 * or for whatever reason sends triplets with multiple channels
+		 * separated when in fact they should be together.
+		 */
+		end_channel = max_subband_chan(cur_channel,
+					       end_channel,
+					       triplet->chans.max_power,
+					       &country_ie,
+					       &country_ie_len);
+		if (!end_channel)
+			return NULL;
+
 		cur_sub_max_channel = end_channel;
 
 		/* Basic sanity check */
@@ -600,10 +799,13 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 
 		last_sub_max_channel = cur_sub_max_channel;
 
-		country_ie += 3;
-		country_ie_len -= 3;
 		num_rules++;
 
+		if (country_ie_len >= 3) {
+			country_ie += 3;
+			country_ie_len -= 3;
+		}
+
 		/*
 		 * Note: this is not a IEEE requirement but
 		 * simply a memory requirement
@@ -646,6 +848,12 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 			continue;
 		}
 
+		if (triplet->chans.first_channel == 0) {
+			country_ie++;
+			country_ie_len--;
+			break;
+		}
+
 		reg_rule = &rd->reg_rules[i];
 		freq_range = &reg_rule->freq_range;
 		power_rule = &reg_rule->power_rule;
@@ -660,6 +868,12 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 			end_channel =  triplet->chans.first_channel +
 				(4 * (triplet->chans.num_channels - 1));
 
+		end_channel = max_subband_chan(triplet->chans.first_channel,
+					       end_channel,
+					       triplet->chans.max_power,
+					       &country_ie,
+					       &country_ie_len);
+
 		/*
 		 * The +10 is since the regulatory domain expects
 		 * the actual band edge, not the center of freq for
@@ -682,10 +896,13 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 		power_rule->max_antenna_gain = DBI_TO_MBI(100);
 		power_rule->max_eirp = DBM_TO_MBM(triplet->chans.max_power);
 
-		country_ie += 3;
-		country_ie_len -= 3;
 		i++;
 
+		if (country_ie_len >= 3) {
+			country_ie += 3;
+			country_ie_len -= 3;
+		}
+
 		BUG_ON(i > NL80211_MAX_SUPP_REG_RULES);
 	}
 
-- 
1.6.3.3