[PATCH] drivers/net/wireless/d80211: Check configuration type in hw->config_interface.

[PATCH] drivers/net/wireless/d80211: Check configuration type in hw->config_interface.

[Jean-Mickael Guerin]

Hello,

This patch prevents a NULL pointer dereferencing in AP mode:
ieee80211_if_config will set conf->bssid only if device is of type STA 
or IBSS.
I see it using following commands right after module loading (with rt61)
# iwconfig wlan0 mode Master
# ifconfig wlan0 up


Signed-off-by: Jean-Mickael Guerin <jean-mickael.guerin@6wind.com>


 adm8211/adm8211.c  |    4 +++-
 rt2x00/rt2400pci.c |    4 +++-
 rt2x00/rt2500pci.c |    4 +++-
 rt2x00/rt61pci.c   |    4 +++-
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/d80211/adm8211/adm8211.c 
b/drivers/net/wireless/d80211/adm8211/adm8211.c
index 9fc5da7..53f05c2 100644
--- a/drivers/net/wireless/d80211/adm8211/adm8211.c
+++ b/drivers/net/wireless/d80211/adm8211/adm8211.c
@@ -1469,7 +1469,9 @@ static int adm8211_config_interface(stru
 {
     struct adm8211_priv *priv = ieee80211_dev_hw_data(dev);
 
-    if (memcmp(conf->bssid, priv->bssid, ETH_ALEN)) {
+    if ((conf->type == IEEE80211_IF_TYPE_STA ||
+         conf->type == IEEE80211_IF_TYPE_IBSS) &&
+         memcmp(conf->bssid, priv->bssid, ETH_ALEN)) {
         adm8211_set_bssid(dev, conf->bssid);
         memcpy(priv->bssid, conf->bssid, ETH_ALEN);
     }
diff --git a/drivers/net/wireless/d80211/rt2x00/rt2400pci.c 
b/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
index 946cf86..1d45851 100644
--- a/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
+++ b/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
@@ -1877,7 +1877,9 @@ rt2400pci_config_interface(struct net_de
     if (rt2x00pci->type == IEEE80211_IF_TYPE_MNTR)
         return 0;
 
-    rt2400pci_config_bssid(rt2x00pci, conf->bssid);
+    if (conf->type == IEEE80211_IF_TYPE_STA ||
+        conf->type == IEEE80211_IF_TYPE_IBSS)
+        rt2400pci_config_bssid(rt2x00pci, conf->bssid);
 
     return 0;
 }
diff --git a/drivers/net/wireless/d80211/rt2x00/rt2500pci.c 
b/drivers/net/wireless/d80211/rt2x00/rt2500pci.c
index ca0edd5..8d2b3a7 100644
--- a/drivers/net/wireless/d80211/rt2x00/rt2500pci.c
+++ b/drivers/net/wireless/d80211/rt2x00/rt2500pci.c
@@ -2000,7 +2000,9 @@ rt2500pci_config_interface(struct net_de
     if (conf->type == IEEE80211_IF_TYPE_MNTR)
         return 0;
 
-    rt2500pci_config_bssid(rt2x00pci, conf->bssid);
+    if (conf->type == IEEE80211_IF_TYPE_STA ||
+        conf->type == IEEE80211_IF_TYPE_IBSS)
+        rt2500pci_config_bssid(rt2x00pci, conf->bssid);
 
     return 0;
 }
diff --git a/drivers/net/wireless/d80211/rt2x00/rt61pci.c 
b/drivers/net/wireless/d80211/rt2x00/rt61pci.c
index 0799f9f..47b2eaf 100644
--- a/drivers/net/wireless/d80211/rt2x00/rt61pci.c
+++ b/drivers/net/wireless/d80211/rt2x00/rt61pci.c
@@ -2463,7 +2463,9 @@ rt61pci_config_interface(struct net_devi
     if (conf->type == IEEE80211_IF_TYPE_MNTR)
         return 0;
 
-    rt61pci_config_bssid(rt2x00pci, conf->bssid);
+    if (conf->type == IEEE80211_IF_TYPE_STA ||
+        conf->type == IEEE80211_IF_TYPE_IBSS)
+        rt61pci_config_bssid(rt2x00pci, conf->bssid);
 
     return 0;
 }

Re: drivers/net/wireless/d80211: Check configuration type in hw->config_interface.

[Ivo Van Doorn]

Hi,

> This patch prevents a NULL pointer dereferencing in AP mode:
> ieee80211_if_config will set conf->bssid only if device is of type STA
> or IBSS.
> I see it using following commands right after module loading (with rt61)
> # iwconfig wlan0 mode Master
> # ifconfig wlan0 up

When submitting a patch, please state what drivers you are really changing,
your mail subject suggested a change to the dscape stack itself.
Since I look at those patches at an irregular basis, while I am always
checking for rt2x00 related patches I could have missed this one.
Especially when you don't CC the driver maintainers about the patch
for their drivers.
Also I think most people would prefer if you split up patches when it
affects multiple drivers,
in this case rt2x00 and adm8211.

As for the patch itself, see my comments below. ;)

> diff --git a/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
> b/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
> index 946cf86..1d45851 100644
> --- a/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
> +++ b/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
> @@ -1877,7 +1877,9 @@ rt2400pci_config_interface(struct net_de
>      if (rt2x00pci->type == IEEE80211_IF_TYPE_MNTR)
>          return 0;
>
> -    rt2400pci_config_bssid(rt2x00pci, conf->bssid);
> +    if (conf->type == IEEE80211_IF_TYPE_STA ||
> +        conf->type == IEEE80211_IF_TYPE_IBSS)
> +        rt2400pci_config_bssid(rt2x00pci, conf->bssid);

Should rt2400pci_config_bssid not simply run a check to see if the
bssid argument is valid?
This would prevent the risk of having a similar problem when the
function is called from somewhere else as well.

Same comment applies for rt2500pci and rt61pci.

Any particular reason why you applied this change to PCI drivers in
rt2x00 only and not to the USB drivers?

Ivo

Re: drivers/net/wireless/d80211: Check configuration type in hw->config_interface.

[Jean-Mickael Guerin]

>
> When submitting a patch, please state what drivers you are really 
> changing,
> your mail subject suggested a change to the dscape stack itself.
> Since I look at those patches at an irregular basis, while I am always
> checking for rt2x00 related patches I could have missed this one.
> Especially when you don't CC the driver maintainers about the patch
> for their drivers.

> Also I think most people would prefer if you split up patches when it
> affects multiple drivers,
> in this case rt2x00 and adm8211.
what I meant with this title is this patch is for all d80211-based 
current drivers of Linville's wireless-dev,
but I see your point...

>
>> diff --git a/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
>> b/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
>> index 946cf86..1d45851 100644
>> --- a/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
>> +++ b/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
>> @@ -1877,7 +1877,9 @@ rt2400pci_config_interface(struct net_de
>>      if (rt2x00pci->type == IEEE80211_IF_TYPE_MNTR)
>>          return 0;
>>
>> -    rt2400pci_config_bssid(rt2x00pci, conf->bssid);
>> +    if (conf->type == IEEE80211_IF_TYPE_STA ||
>> +        conf->type == IEEE80211_IF_TYPE_IBSS)
>> +        rt2400pci_config_bssid(rt2x00pci, conf->bssid);
>
> Should rt2400pci_config_bssid not simply run a check to see if the
> bssid argument is valid?
> This would prevent the risk of having a similar problem when the
> function is called from somewhere else as well.
>
I was thinking a function named xxx_config_bssid() assumes a valid bssid 
pointer,
- I would even add BUG_ON(conf->bssid==NULL) in xxx_config_bssid().
And hw->interface already already some tests with conf->type.
And net/d80211 uses same kind of test before setting conf->bssid.

anyway I don't mind you make the patch the other way.
> Same comment applies for rt2500pci and rt61pci.
>
> Any particular reason why you applied this change to PCI drivers in
> rt2x00 only and not to the USB drivers?
>
likely because I have only a pci card :-)
USB drivers needs same change too.

Thanks,

Jean-Mickael

Re: [PATCH] drivers/net/wireless/d80211: Check configuration type in hw->config_interface.

[Michael Wu]

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

On Wednesday 19 July 2006 13:26, Jean-Mickael Guerin wrote:
> This patch prevents a NULL pointer dereferencing in AP mode:
> ieee80211_if_config will set conf->bssid only if device is of type STA
> or IBSS.
Why is that? Isn't there a BSSID in AP mode too? Perhaps it is calling 
config_interface before setting the BSSID?

adm8211 doesn't support AP mode yet, but it's good to know this crash won't 
occur when it does. :)

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] drivers/net/wireless/d80211: Check configuration type in hw->config_interface.

[Jiri Benc]

On Wed, 19 Jul 2006 18:07:05 -0700, Michael Wu wrote:
> Why is that? Isn't there a BSSID in AP mode too? Perhaps it is calling 
> config_interface before setting the BSSID?

The bssid field in ieee80211_if_conf structure is not set in AP mode.
There is no need for that - you already have a MAC address of the AP
interface (from add_interface callback). That's your BSSID.

> adm8211 doesn't support AP mode yet, but it's good to know this crash won't 
> occur when it does. :)

The crash won't occur even without the patch - you will need to do
completely different things in adm8211_config_interface for AP mode than
for STA or IBSS mode and you will put some switch there anyway. No
reason for doing it now and bloating the code with a check for condition
that cannot happen.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] drivers/net/wireless/d80211: Check configuration type in hw->config_interface.

[Jiri Benc]

On Wed, 19 Jul 2006 22:26:52 +0200, Jean-Mickael Guerin wrote:
> This patch prevents a NULL pointer dereferencing in AP mode:
> ieee80211_if_config will set conf->bssid only if device is of type STA 
> or IBSS.
> I see it using following commands right after module loading (with rt61)
> # iwconfig wlan0 mode Master
> # ifconfig wlan0 up

The patch seems to fix the problem at a wrong place. rt2x00 has broken
add_interface handler - it allows adding of AP interface even though the
driver doesn't support AP mode. It is add_interface callback that should
be fixed in rt2x00.

The check in the patch most likely won't be needed even after AP mode
support is added to rt2x00 - the driver needs to handle AP mode
differently so config_interface callback will be rewritten anyway.

adm8211 driver doesn't have this problem and doesn't need to be
modified.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] drivers/net/wireless/d80211: Check configuration type in hw->config_interface.

[Ivo Van Doorn]

Hi,

> > This patch prevents a NULL pointer dereferencing in AP mode:
> > ieee80211_if_config will set conf->bssid only if device is of type STA
> > or IBSS.
> > I see it using following commands right after module loading (with rt61)
> > # iwconfig wlan0 mode Master
> > # ifconfig wlan0 up
>
> The patch seems to fix the problem at a wrong place. rt2x00 has broken
> add_interface handler - it allows adding of AP interface even though the
> driver doesn't support AP mode. It is add_interface callback that should
> be fixed in rt2x00.

Well rt2x00 does support AP mode, our latest CVS tree (patches for
wireless-dev are in progress) has even shown a working configuration
for some users.
So add_interface is correct at allowing the AP interface, perhaps some
more steps
are required to make it completely work, but it is work in progress.

> The check in the patch most likely won't be needed even after AP mode
> support is added to rt2x00 - the driver needs to handle AP mode
> differently so config_interface callback will be rewritten anyway.

I'll make a check to see if the bssid is NULL or invalid in the
config_bssid() function,
and make sure that in AP mode the MAC is written as BSSID.

Ivo

Re: [PATCH] drivers/net/wireless/d80211: Check configuration type in hw->config_interface.

[Ivo Van Doorn]

Hi,

> > > This patch prevents a NULL pointer dereferencing in AP mode:
> > > ieee80211_if_config will set conf->bssid only if device is of type STA
> > > or IBSS.
> > > I see it using following commands right after module loading (with rt61)
> > > # iwconfig wlan0 mode Master
> > > # ifconfig wlan0 up
> >
> > The patch seems to fix the problem at a wrong place. rt2x00 has broken
> > add_interface handler - it allows adding of AP interface even though the
> > driver doesn't support AP mode. It is add_interface callback that should
> > be fixed in rt2x00.
>
> Well rt2x00 does support AP mode, our latest CVS tree (patches for
> wireless-dev are in progress) has even shown a working configuration
> for some users.
> So add_interface is correct at allowing the AP interface, perhaps some
> more steps
> are required to make it completely work, but it is work in progress.
>
> > The check in the patch most likely won't be needed even after AP mode
> > support is added to rt2x00 - the driver needs to handle AP mode
> > differently so config_interface callback will be rewritten anyway.
>
> I'll make a check to see if the bssid is NULL or invalid in the
> config_bssid() function,
> and make sure that in AP mode the MAC is written as BSSID.

I won't be able to send this patch seperately, so it will become part
of the larger series
that I am currently working on, that patch series throws a lot of code
around and has some major changes to the rt2x00 code so expect very
large patches.
The bssid is NULL fix was already in my cvs tree,
so only the update to use the MAC as BSSID in master mode has been added.

Ivo