[PATCH] mac80211: don't allow zero mac bssid to be configured

[PATCH] mac80211: don't allow zero mac bssid to be configured

[Marek Lindner]

Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 net/mac80211/ibss.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 4f9235b..9c7991e 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -588,12 +588,10 @@ static void ieee80211_sta_find_ibss(struct ieee80211_sub_if_data *sdata)
 	capability = WLAN_CAPABILITY_IBSS;
 	if (ifibss->privacy)
 		capability |= WLAN_CAPABILITY_PRIVACY;
-	if (ifibss->fixed_bssid)
+	if (ifibss->fixed_bssid && !is_zero_ether_addr(ifibss->bssid))
 		bssid = ifibss->bssid;
 	if (ifibss->fixed_channel)
 		chan = ifibss->channel;
-	if (!is_zero_ether_addr(ifibss->bssid))
-		bssid = ifibss->bssid;
 	cbss = cfg80211_get_bss(local->hw.wiphy, chan, bssid,
 				ifibss->ssid, ifibss->ssid_len,
 				WLAN_CAPABILITY_IBSS | WLAN_CAPABILITY_PRIVACY,
-- 
1.7.5.4

Re: [PATCH] mac80211: don't allow zero mac bssid to be configured

[Johannes Berg]

How is this related to configuration? Shouldn't the configuration check
be in cfg80211 instead?

johannes


On Thu, 2011-09-15 at 18:07 +0200, Marek Lindner wrote:
> Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
> ---
>  net/mac80211/ibss.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
> index 4f9235b..9c7991e 100644
> --- a/net/mac80211/ibss.c
> +++ b/net/mac80211/ibss.c
> @@ -588,12 +588,10 @@ static void ieee80211_sta_find_ibss(struct ieee80211_sub_if_data *sdata)
>  	capability = WLAN_CAPABILITY_IBSS;
>  	if (ifibss->privacy)
>  		capability |= WLAN_CAPABILITY_PRIVACY;
> -	if (ifibss->fixed_bssid)
> +	if (ifibss->fixed_bssid && !is_zero_ether_addr(ifibss->bssid))
>  		bssid = ifibss->bssid;
>  	if (ifibss->fixed_channel)
>  		chan = ifibss->channel;
> -	if (!is_zero_ether_addr(ifibss->bssid))
> -		bssid = ifibss->bssid;
>  	cbss = cfg80211_get_bss(local->hw.wiphy, chan, bssid,
>  				ifibss->ssid, ifibss->ssid_len,
>  				WLAN_CAPABILITY_IBSS | WLAN_CAPABILITY_PRIVACY,

Re: [PATCH] mac80211: don't allow zero mac bssid to be configured

[Marek Lindner]

On Thursday, September 15, 2011 18:21:38 Johannes Berg wrote:
> How is this related to configuration? 

These 'sdata->u.ibss' values are initialized by ieee80211_ibss_join() which is 
called when a user invokes "iw dev wlan0 ibss join".


> Shouldn't the configuration check be in cfg80211 instead?

No idea. I just stumbled over this obvious bug when I scrolled down the code. 
Feel free to propose an alternative patch if you have a better solution.

Cheers,
Marek

Re: [PATCH] mac80211: don't allow zero mac bssid to be configured

[Johannes Berg]

On Fri, 2011-09-16 at 05:14 +0200, Marek Lindner wrote:
> On Thursday, September 15, 2011 18:21:38 Johannes Berg wrote:
> > How is this related to configuration? 
> 
> These 'sdata->u.ibss' values are initialized by ieee80211_ibss_join() which is 
> called when a user invokes "iw dev wlan0 ibss join".

Right. So that means you shouldn't be rejecting the configuration when
actually using it. You should be rejecting it when it's actually set.

> > Shouldn't the configuration check be in cfg80211 instead?
> 
> No idea. I just stumbled over this obvious bug when I scrolled down the code. 
> Feel free to propose an alternative patch if you have a better solution.

It should be in cfg80211, so NACK for this patch. Also, this patch is
wrong -- at that point, checking for zero address serves a difference
purpose I think, it means more something along the lines of "did we
previously find an IBSS".

johannes

Re: [PATCH] mac80211: don't allow zero mac bssid to be configured

[Marek Lindner]

On Friday, September 16, 2011 08:19:16 Johannes Berg wrote:
> On Fri, 2011-09-16 at 05:14 +0200, Marek Lindner wrote:
> > On Thursday, September 15, 2011 18:21:38 Johannes Berg wrote:
> > > How is this related to configuration?
> > 
> > These 'sdata->u.ibss' values are initialized by ieee80211_ibss_join()
> > which is called when a user invokes "iw dev wlan0 ibss join".
> 
> Right. So that means you shouldn't be rejecting the configuration when
> actually using it. You should be rejecting it when it's actually set.

I'd like to remind you that it was not me who designed the original check (my 
patch does not add anything new). If you are looking for somebody to beautify 
your code you'll have to look somewhere else.


> It should be in cfg80211, so NACK for this patch.

As I said: Feel free to propose an alternative patch. I certainly won't.


> Also, this patch is wrong -- at that point, checking for zero address serves
> a difference purpose I think, it means more something along the lines of
> "did we previously find an IBSS".

Can't agree here. This check was conceived to hinder a fixed-bssid 
configuration of 00:00:00:00:00:00.

Cheers,
Marek

Re: [PATCH] mac80211: don't allow zero mac bssid to be configured

[Johannes Berg]

On Fri, 2011-09-16 at 13:27 +0200, Marek Lindner wrote:

> > Right. So that means you shouldn't be rejecting the configuration when
> > actually using it. You should be rejecting it when it's actually set.
> 
> I'd like to remind you that it was not me who designed the original check (my 
> patch does not add anything new). If you are looking for somebody to beautify 
> your code you'll have to look somewhere else.

If you're looking to fix a problem, I told you how to fix it.
If you're just reporting a bug along with "this is what I think the fix
might be" then maybe you shouldn't take rejection of the proposal so
badly.

I was, maybe erroneously, assuming that you wanted to fix the issue.

> As I said: Feel free to propose an alternative patch. I certainly won't.

Fair enough. I take it you're just reporting a bug then.

> > Also, this patch is wrong -- at that point, checking for zero address serves
> > a difference purpose I think, it means more something along the lines of
> > "did we previously find an IBSS".
> 
> Can't agree here. This check was conceived to hinder a fixed-bssid 
> configuration of 00:00:00:00:00:00.

You just said that you didn't originally design it so how do you know
the design intent? :-)

FWIW, I think the check there pre-dates the current cfg80211/mac80211
design. The current assumption in mac80211 clearly is that ifibss->bssid
is *always* valid since it's set to a random address when asked to join.
Hence the only way it can be invalid is the bug case you reported (join
fixed zeroes BSSID).

Anyway. The right fix remains in cfg80211, and the is_zero_ether_addr()
check in mac80211 can be deleted as it is completely pointless since
it'll never be false.

johannes

Re: [PATCH] mac80211: don't allow zero mac bssid to be configured

[Johannes Berg]

On Fri, 2011-09-16 at 13:41 +0200, Johannes Berg wrote:

> FWIW, I think the check there pre-dates the current cfg80211/mac80211
> design. The current assumption in mac80211 clearly is that ifibss->bssid
> is *always* valid since it's set to a random address when asked to join.
> Hence the only way it can be invalid is the bug case you reported (join
> fixed zeroes BSSID).

Actually, that's not true. And in fact, setting a fixed BSSID with a
zero address will cause some funny things to happen, but not necessarily
what you think :-)

The is_zero_ether_addr() check there means "have we found an IBSS
already?" and as such must stay. It doesn't mean "do we have a valid
configuration" at all...

johannes

Re: [PATCH] mac80211: don't allow zero mac bssid to be configured

[Marek Lindner]

On Friday, September 16, 2011 13:49:05 Johannes Berg wrote:
> Fair enough. I take it you're just reporting a bug then.

I can live with that.


> The is_zero_ether_addr() check there means "have we found an IBSS
> already?" and as such must stay. It doesn't mean "do we have a valid
> configuration" at all...

In that case you probably should add a comment to make it obvious or come up 
with a cleaner approach of implementing "have we found an IBSS already?".
But who I am to judge the code ?  :-)

Regards,
Marek