[PATCH] cfg80211: fix cmp_hidden_bss

[PATCH] cfg80211: fix cmp_hidden_bss

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The cmp_bss() comparator function uses memcmp() to
compare the SSID. This means that cmp_hidden_bss()
needs to similarly return a number bigger than zero
(use 1) instead of -1 when ie1 is bigger than ie2,
which is the case if an ie2 byte is non-zero.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 9233910..57855bc 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -472,7 +472,7 @@ static int cmp_hidden_bss(struct cfg80211_bss *a, struct cfg80211_bss *b)
 	/* zeroed SSID ie is another indication of a hidden bss */
 	for (i = 0; i < ie2[1]; i++)
 		if (ie2[i + 2])
-			return -1;
+			return 1;
 
 	return 0;
 }
-- 
1.8.0

Re: [PATCH] cfg80211: fix cmp_hidden_bss

[Eliad Peller]

On Wed, Nov 28, 2012 at 11:44 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> The cmp_bss() comparator function uses memcmp() to
> compare the SSID. This means that cmp_hidden_bss()
> needs to similarly return a number bigger than zero
> (use 1) instead of -1 when ie1 is bigger than ie2,
> which is the case if an ie2 byte is non-zero.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  net/wireless/scan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index 9233910..57855bc 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -472,7 +472,7 @@ static int cmp_hidden_bss(struct cfg80211_bss *a, struct cfg80211_bss *b)
>         /* zeroed SSID ie is another indication of a hidden bss */
>         for (i = 0; i < ie2[1]; i++)
>                 if (ie2[i + 2])
> -                       return -1;
> +                       return 1;
>
>         return 0;
>  }

maybe i'm misreading it, but there is still no comparison here at all,
so it seems wrong as well :)
(practically, i guess it doesn't really matter, because if there is
some non-null char, this is not a hidden ssid anyway)

Eliad.

Re: [PATCH] cfg80211: fix cmp_hidden_bss

[Johannes Berg]

On Thu, 2012-11-29 at 01:53 +0200, Eliad Peller wrote:
> On Wed, Nov 28, 2012 at 11:44 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > The cmp_bss() comparator function uses memcmp() to
> > compare the SSID. This means that cmp_hidden_bss()
> > needs to similarly return a number bigger than zero
> > (use 1) instead of -1 when ie1 is bigger than ie2,
> > which is the case if an ie2 byte is non-zero.

> > +++ b/net/wireless/scan.c
> > @@ -472,7 +472,7 @@ static int cmp_hidden_bss(struct cfg80211_bss *a, struct cfg80211_bss *b)
> >         /* zeroed SSID ie is another indication of a hidden bss */
> >         for (i = 0; i < ie2[1]; i++)
> >                 if (ie2[i + 2])
> > -                       return -1;
> > +                       return 1;
> >
> >         return 0;
> >  }
> 
> maybe i'm misreading it, but there is still no comparison here at all,
> so it seems wrong as well :)
> (practically, i guess it doesn't really matter, because if there is
> some non-null char, this is not a hidden ssid anyway)

Hmm yeah you're right, it should be
	return ie2[i + 1] - ie1[i + 1];

(or the other way around? need to sleep on that first...)

johannes

Re: [PATCH] cfg80211: fix cmp_hidden_bss

[Eliad Peller]

On Thu, Nov 29, 2012 at 2:00 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2012-11-29 at 01:53 +0200, Eliad Peller wrote:
>> On Wed, Nov 28, 2012 at 11:44 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > From: Johannes Berg <johannes.berg@intel.com>
>> >
>> > The cmp_bss() comparator function uses memcmp() to
>> > compare the SSID. This means that cmp_hidden_bss()
>> > needs to similarly return a number bigger than zero
>> > (use 1) instead of -1 when ie1 is bigger than ie2,
>> > which is the case if an ie2 byte is non-zero.
>
>> > +++ b/net/wireless/scan.c
>> > @@ -472,7 +472,7 @@ static int cmp_hidden_bss(struct cfg80211_bss *a, struct cfg80211_bss *b)
>> >         /* zeroed SSID ie is another indication of a hidden bss */
>> >         for (i = 0; i < ie2[1]; i++)
>> >                 if (ie2[i + 2])
>> > -                       return -1;
>> > +                       return 1;
>> >
>> >         return 0;
>> >  }
>>
>> maybe i'm misreading it, but there is still no comparison here at all,
>> so it seems wrong as well :)
>> (practically, i guess it doesn't really matter, because if there is
>> some non-null char, this is not a hidden ssid anyway)
>
> Hmm yeah you're right, it should be
>         return ie2[i + 1] - ie1[i + 1];
>
> (or the other way around? need to sleep on that first...)
>
that's also not enough, because you ignore the previous chars.
("A" < "B" vs. "AA" > "\x00B")

i think returning memcmp() is the easiest solution, but sleeping on
that is definitely a good idea :)

Eliad.

Re: [PATCH] cfg80211: fix cmp_hidden_bss

[Johannes Berg]

On Thu, 2012-11-29 at 02:07 +0200, Eliad Peller wrote:
> On Thu, Nov 29, 2012 at 2:00 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Thu, 2012-11-29 at 01:53 +0200, Eliad Peller wrote:
> >> On Wed, Nov 28, 2012 at 11:44 PM, Johannes Berg
> >> <johannes@sipsolutions.net> wrote:
> >> > From: Johannes Berg <johannes.berg@intel.com>
> >> >
> >> > The cmp_bss() comparator function uses memcmp() to
> >> > compare the SSID. This means that cmp_hidden_bss()
> >> > needs to similarly return a number bigger than zero
> >> > (use 1) instead of -1 when ie1 is bigger than ie2,
> >> > which is the case if an ie2 byte is non-zero.
> >
> >> > +++ b/net/wireless/scan.c
> >> > @@ -472,7 +472,7 @@ static int cmp_hidden_bss(struct cfg80211_bss *a, struct cfg80211_bss *b)
> >> >         /* zeroed SSID ie is another indication of a hidden bss */
> >> >         for (i = 0; i < ie2[1]; i++)
> >> >                 if (ie2[i + 2])
> >> > -                       return -1;
> >> > +                       return 1;
> >> >
> >> >         return 0;
> >> >  }
> >>
> >> maybe i'm misreading it, but there is still no comparison here at all,
> >> so it seems wrong as well :)
> >> (practically, i guess it doesn't really matter, because if there is
> >> some non-null char, this is not a hidden ssid anyway)
> >
> > Hmm yeah you're right, it should be
> >         return ie2[i + 1] - ie1[i + 1];
> >
> > (or the other way around? need to sleep on that first...)
> >
> that's also not enough, because you ignore the previous chars.
> ("A" < "B" vs. "AA" > "\x00B")
> 
> i think returning memcmp() is the easiest solution, but sleeping on
> that is definitely a good idea :)

Hmm, yeah, indeed. If we find a non-zero byte we should return memcmp().
I'll fix it in the morning :)

johannes

[PATCH v2] cfg80211: fix cmp_hidden_bss

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The cmp_bss() comparator function uses memcmp() to
compare the SSID. This means that cmp_hidden_bss()
needs to similarly return a number bigger than zero
(use 1) instead of -1 when ie1 is bigger than ie2,
which is the case if an ie2 byte is non-zero.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/scan.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 9233910..456c342 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -469,10 +469,14 @@ static int cmp_hidden_bss(struct cfg80211_bss *a, struct cfg80211_bss *b)
 	if (ie1[1] != ie2[1])
 		return ie2[1] - ie1[1];
 
-	/* zeroed SSID ie is another indication of a hidden bss */
+	/*
+	 * zeroed SSID ie is another indication of a hidden bss;
+	 * if it isn't zeroed just return the regular sort value
+	 * to find the next candidate
+	 */
 	for (i = 0; i < ie2[1]; i++)
 		if (ie2[i + 2])
-			return -1;
+			return memcmp(ie1 + 2, ie2 + 2, ie1[1]);
 
 	return 0;
 }
-- 
1.8.0

Re: [PATCH v2] cfg80211: fix cmp_hidden_bss

[Eliad Peller]

On Thu, Nov 29, 2012 at 11:01 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> The cmp_bss() comparator function uses memcmp() to
> compare the SSID. This means that cmp_hidden_bss()
> needs to similarly return a number bigger than zero
> (use 1) instead of -1 when ie1 is bigger than ie2,
> which is the case if an ie2 byte is non-zero.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  net/wireless/scan.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index 9233910..456c342 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -469,10 +469,14 @@ static int cmp_hidden_bss(struct cfg80211_bss *a, struct cfg80211_bss *b)
>         if (ie1[1] != ie2[1])
>                 return ie2[1] - ie1[1];
>
> -       /* zeroed SSID ie is another indication of a hidden bss */
> +       /*
> +        * zeroed SSID ie is another indication of a hidden bss;
> +        * if it isn't zeroed just return the regular sort value
> +        * to find the next candidate
> +        */
>         for (i = 0; i < ie2[1]; i++)
>                 if (ie2[i + 2])
> -                       return -1;
> +                       return memcmp(ie1 + 2, ie2 + 2, ie1[1]);
>
>         return 0;
>  }

btw, it might be worth noting that this comparator works correctly
only in one way (if hidden ssid is in "b")...

Eliad.

[PATCH v3] cfg80211: fix cmp_hidden_bss

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The cmp_bss() comparator function uses memcmp() to
compare the SSID. This means that cmp_hidden_bss()
needs to similarly return a number bigger than zero
(use 1) instead of -1 when ie1 is bigger than ie2,
which is the case if an ie2 byte is non-zero.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/scan.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 9233910..834e0d1 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -450,10 +450,16 @@ static int cmp_hidden_bss(struct cfg80211_bss *a, struct cfg80211_bss *b)
 			       b->information_elements,
 			       b->len_information_elements);
 
-	/* Key comparator must use same algorithm in any rb-tree
+	/*
+	 * Key comparator must use same algorithm in any rb-tree
 	 * search function (order is important), otherwise ordering
 	 * of items in the tree is broken and search gives incorrect
-	 * results. This code uses same order as cmp_ies() does. */
+	 * results. This code uses same order as cmp_ies() does.
+	 *
+	 * Note that due to the differring behaviour with hidden SSIDs
+	 * this function only works when "b" is the tree element and
+	 * "a" is the key we're looking for.
+	 */
 
 	/* sort missing IE before (left of) present IE */
 	if (!ie1)
@@ -469,10 +475,14 @@ static int cmp_hidden_bss(struct cfg80211_bss *a, struct cfg80211_bss *b)
 	if (ie1[1] != ie2[1])
 		return ie2[1] - ie1[1];
 
-	/* zeroed SSID ie is another indication of a hidden bss */
+	/*
+	 * zeroed SSID ie is another indication of a hidden bss;
+	 * if it isn't zeroed just return the regular sort value
+	 * to find the next candidate
+	 */
 	for (i = 0; i < ie2[1]; i++)
 		if (ie2[i + 2])
-			return -1;
+			return memcmp(ie1 + 2, ie2 + 2, ie1[1]);
 
 	return 0;
 }
-- 
1.8.0

Re: [PATCH v3] cfg80211: fix cmp_hidden_bss

[Johannes Berg]

On Thu, 2012-11-29 at 11:48 +0100, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> The cmp_bss() comparator function uses memcmp() to
> compare the SSID. This means that cmp_hidden_bss()
> needs to similarly return a number bigger than zero
> (use 1) instead of -1 when ie1 is bigger than ie2,
> which is the case if an ie2 byte is non-zero.

applied.

johannes