[PATCH 1/2] cfg80211: reject invalid IBSS BSSIDs in wext compat code

[PATCH 1/2] cfg80211: reject invalid IBSS BSSIDs in wext compat code

[Johannes Berg]

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

Don't allow using a multicast address as the BSSID, that
isn't a valid configuration.

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

diff --git a/net/wireless/ibss.c b/net/wireless/ibss.c
index 071e99c75e82..a953a60b4f41 100644
--- a/net/wireless/ibss.c
+++ b/net/wireless/ibss.c
@@ -481,6 +481,9 @@ int cfg80211_ibss_wext_siwap(struct net_device *dev,
 	if (is_zero_ether_addr(bssid) || is_broadcast_ether_addr(bssid))
 		bssid = NULL;
 
+	if (bssid && !is_valid_ether_addr(bssid))
+		return -EINVAL;
+
 	/* both automatic */
 	if (!bssid && !wdev->wext.ibss.bssid)
 		return 0;
-- 
1.9.0

[PATCH 2/2] cfg80211: ignore invalid BSSIDs when looking for BSSes

[Johannes Berg]

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

When looking for a BSS matching given parameters, ignore invalid
BSSIDs. This avoids, for example, trying to join an IBSS that has
a multicast BSSID, which isn't supported by all drivers nor is it
a valid configuration of the IBSS so better create a new one with
a correctly chosen random BSSID.

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

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 7d09a712cb1f..746c56ebd66c 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -540,6 +540,8 @@ struct cfg80211_bss *cfg80211_get_bss(struct wiphy *wiphy,
 			continue;
 		if (channel && bss->pub.channel != channel)
 			continue;
+		if (!is_valid_ether_addr(bss->pub.bssid))
+			continue;
 		/* Don't get expired BSS structs */
 		if (time_after(now, bss->ts + IEEE80211_SCAN_RESULT_EXPIRE) &&
 		    !atomic_read(&bss->hold))
-- 
1.9.0

Re: [PATCH 2/2] cfg80211: ignore invalid BSSIDs when looking for BSSes

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On 09/04/14 22:39, Johannes Berg wrote:
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index 7d09a712cb1f..746c56ebd66c 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -540,6 +540,8 @@ struct cfg80211_bss *cfg80211_get_bss(struct wiphy *wiphy,
>  			continue;
>  		if (channel && bss->pub.channel != channel)
>  			continue;
> +		if (!is_valid_ether_addr(bss->pub.bssid))
> +			continue;

Wouldn't it be better to prevent such entry to end up in the bss list at
all? (i.e. filtering during the scan?)


Cheers,


-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH 2/2] cfg80211: ignore invalid BSSIDs when looking for BSSes

[Johannes Berg]

On Thu, 2014-04-10 at 09:00 +0200, Antonio Quartulli wrote:
> On 09/04/14 22:39, Johannes Berg wrote:
> > diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> > index 7d09a712cb1f..746c56ebd66c 100644
> > --- a/net/wireless/scan.c
> > +++ b/net/wireless/scan.c
> > @@ -540,6 +540,8 @@ struct cfg80211_bss *cfg80211_get_bss(struct wiphy *wiphy,
> >  			continue;
> >  		if (channel && bss->pub.channel != channel)
> >  			continue;
> > +		if (!is_valid_ether_addr(bss->pub.bssid))
> > +			continue;
> 
> Wouldn't it be better to prevent such entry to end up in the bss list at
> all? (i.e. filtering during the scan?)

I thought about that, but I'm not so sure.

On the one hand, that would make sense since it's unusable, on the other
hand it could cause confusion if you don't see some other network that
you expect to show up ...

And then again, if you can click the network in the UI and then it won't
connect to it, that also causes confusion. I'm not sure what the best
way is ...

johannes

Re: [PATCH 2/2] cfg80211: ignore invalid BSSIDs when looking for BSSes

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]



On 10/04/14 09:59, Johannes Berg wrote:
> On Thu, 2014-04-10 at 09:00 +0200, Antonio Quartulli wrote:
>> On 09/04/14 22:39, Johannes Berg wrote:
>>> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
>>> index 7d09a712cb1f..746c56ebd66c 100644
>>> --- a/net/wireless/scan.c
>>> +++ b/net/wireless/scan.c
>>> @@ -540,6 +540,8 @@ struct cfg80211_bss *cfg80211_get_bss(struct wiphy *wiphy,
>>>  			continue;
>>>  		if (channel && bss->pub.channel != channel)
>>>  			continue;
>>> +		if (!is_valid_ether_addr(bss->pub.bssid))
>>> +			continue;
>>
>> Wouldn't it be better to prevent such entry to end up in the bss list at
>> all? (i.e. filtering during the scan?)
> 
> I thought about that, but I'm not so sure.
> 
> On the one hand, that would make sense since it's unusable, on the other
> hand it could cause confusion if you don't see some other network that
> you expect to show up ...

I agree here.
Showing the network is reasonable because the user expects to see what's
around. Imagine a user that scans just to see which channel is less
crowded...

> 
> And then again, if you can click the network in the UI and then it won't
> connect to it, that also causes confusion. I'm not sure what the best
> way is ...
> 

True. But here it's up to the UI to check the MAC and tell the user that
this is an invalid BSSID, no?

In the end I drop my original objection :)



Cheers,

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]