diff mbox

Fix a channel-to-frequency transformation in ieee80211_chan_to_freq()

Message ID CAAYDh_gV6Eb7AB0dW2xWEy2GiFU8Ru7v-PwuizVn8wFhC2YTFg@mail.gmail.com
State Superseded
Headers show

Commit Message

SiWon Kang Feb. 19, 2017, 3:35 p.m. UTC 
>> - int freq;
>> + if (country_match(us_op_class_cc, country))
>
> In addition, this is whitespace damaged (tabs converted to a single
> space), so I cannot apply it.

At first, I am very sorry about the whitespace. I should have rather attached
the patch file than copy-and-paste the contents. I used to upload through
git send-email but it is blocked by gmail now. Please kindly use attached
file for the patch work.

> Does this fix something externally observable? It looks like the current
> design of the function uses the global table as a backup path on
> purpose. As such, I would rather not modify this unless there is a
> clearly identifiable misbehavior and/or no chances of breaking some
> existing use cases.

The externally observable malfunction is the set provided in the patch as
an example. As you know well, the US regulatory does not allow ch 12
and 13 so this function shall return -1 if the country code is explicitly
given as 'US' just like that -1 is returned for channel 14 when CC is in the
set of 'us' or 'eu'. However, it currently returns 2467 or 2472 for the input
12 or 13 + US so that it looks those channels are allowed. The header
comments says the return must be -1 if the specified channel is unknown.
Since the channel 12, 13 are unknown in US, it shall return -1.
As well as it shouldn't use global table as a backup path because, as
stated in a doxygen format description, the function uses global op_class
only if the country code is unknown. In above case it specifies the known
country code - US so it should not use global op_class.
/**
 * ieee80211_chan_to_freq - Convert channel info to frequency
 * @country: Country code, if known; otherwise, global operating class is used
 * @op_class: Operating class
 * @chan: Channel number
 * Returns: Frequency in MHz or -1 if the specified channel is unknown
 */

FYI, I am making an API utiliizing this function. If this function
returns positive
values for the channels not allowed in the region, I have to add check routine
for channels' validity which is major advantage of using this function. I'd very
appreciate if you could kindly take your precious time for a careful review.

Comments

Jouni Malinen March 1, 2017, 8:39 a.m. UTC | #1 
On Mon, Feb 20, 2017 at 12:35:08AM +0900, Siwon Kang wrote:
> The externally observable malfunction is the set provided in the patch as
> an example. As you know well, the US regulatory does not allow ch 12
> and 13 so this function shall return -1 if the country code is explicitly
> given as 'US' just like that -1 is returned for channel 14 when CC is in the
> set of 'us' or 'eu'. However, it currently returns 2467 or 2472 for the input
> 12 or 13 + US so that it looks those channels are allowed. The header
> comments says the return must be -1 if the specified channel is unknown.
> Since the channel 12, 13 are unknown in US, it shall return -1.
> As well as it shouldn't use global table as a backup path because, as
> stated in a doxygen format description, the function uses global op_class
> only if the country code is unknown. In above case it specifies the known
> country code - US so it should not use global op_class.

I'd like to understand what actual hostapd or wpa_supplicant
functionality this patch would fix. I cannot really see it fixing
anything in the current functionality. Can you identify a specific
caller of ieee80211_chan_to_freq() that gets fixed if this patch gets
applied?

On the other hand, this would seem to break functionality in one of the
callers, i.e., the one in wnm_nei_get_chan() (wpa_supplicant/wnm_sta.c).
That code fetches the Country element to see which country is in use and
then expect ieee80211_chan_to_freq() to have the fallback option to the
global table if none of the country-specific operating classes matched.
In theory, wnm_nei_get_chan() could be modified to clear the country
pointer back to NULL if the third octet of the country string has 0x04
in it. That said, I'm not convinced that all AP vendors will implement
this correctly and as such, want to have the fallback option to look up
the global table regardless. I guess this function could try to check
for that 0x04 first and then if the country specific lookup fails, fall
back to trying without a country code in this function.

In any case, without addressing all the existing callers (of which I
could identify this wnm_nei_get_chan() being the only problematic one)
in the same patch, I cannot apply this patch since it could result in
regressions to existing functionality.
SiWon Kang March 2, 2017, 3:26 p.m. UTC | #2 
Hi,

Thanks a lot for the detailed explanation.
> In theory, wnm_nei_get_chan() could be modified to clear the country
> pointer back to NULL if the third octet of the country string has 0x04
> in it. That said, I'm not convinced that all AP vendors will implement
> this correctly and as such, want to have the fallback option to look up
> the global table regardless.
Now I fully understand your intention to have this fallback routine and
what was based on that thought. My patch is to filter out the wrong
matches and return failure whereas current design is to accept those
faults done by not-so-skillful AP vendors. We shall drop this patch which
is opposite to your idea.

BR,
Shawn.
diff mbox

Patch

From 8b01ed12f162f2eb08f4734a9771e495822d1cbd Mon Sep 17 00:00:00 2001
From: Siwon Kang <kkangshawn@gmail.com>
Date: Mon, 20 Feb 2017 00:28:49 +0900
Subject: [PATCH] Fix a channel-to-frequency transformation in
 ieee80211_chan_to_freq()

The function ieee80211_chan_to_freq() is supposed to return -1 if the
specified channel is unknown. However, current if-conditional statements
do not catch a few wrong combination. For example, {"US", 81, 13} for
country code, operating class, channel, respectively should
return -1 because op_class 81 is not in the list and channel 13 is not
allowed but currently returns 2472 as if it is global.

This patch removes 'if (freq > 0)' to prevent from stepping over when an
input op_class is out of the list of a country then finally going to
ieee80211_chan_to_freq_global(). Instead, it returns result of
sub-function directly which is correct.

Signed-off-by: Siwon Kang <kkangshawn@gmail.com>
---
 src/common/ieee802_11_common.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/src/common/ieee802_11_common.c b/src/common/ieee802_11_common.c
index a8d68e5..5ba5071 100644
--- a/src/common/ieee802_11_common.c
+++ b/src/common/ieee802_11_common.c
@@ -1133,31 +1133,17 @@  static int ieee80211_chan_to_freq_global(u8 op_class, u8 chan)
  */
 int ieee80211_chan_to_freq(const char *country, u8 op_class, u8 chan)
 {
-	int freq;
+	if (country_match(us_op_class_cc, country))
+		return ieee80211_chan_to_freq_us(op_class, chan);
 
-	if (country_match(us_op_class_cc, country)) {
-		freq = ieee80211_chan_to_freq_us(op_class, chan);
-		if (freq > 0)
-			return freq;
-	}
-
-	if (country_match(eu_op_class_cc, country)) {
-		freq = ieee80211_chan_to_freq_eu(op_class, chan);
-		if (freq > 0)
-			return freq;
-	}
+	if (country_match(eu_op_class_cc, country))
+		return ieee80211_chan_to_freq_eu(op_class, chan);
 
-	if (country_match(jp_op_class_cc, country)) {
-		freq = ieee80211_chan_to_freq_jp(op_class, chan);
-		if (freq > 0)
-			return freq;
-	}
+	if (country_match(jp_op_class_cc, country))
+		return ieee80211_chan_to_freq_jp(op_class, chan);
 
-	if (country_match(cn_op_class_cc, country)) {
-		freq = ieee80211_chan_to_freq_cn(op_class, chan);
-		if (freq > 0)
-			return freq;
-	}
+	if (country_match(cn_op_class_cc, country))
+		return ieee80211_chan_to_freq_cn(op_class, chan);
 
 	return ieee80211_chan_to_freq_global(op_class, chan);
 }
-- 
2.5.5