Skip to contentSkip to conversation

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

PATCH In review Jean-Mickael Guerin 4 participants 8 messages Jul 19, 2006 – Jul 20, 2006

Subsystems (3)
Networking Network drivers Networking drivers (wireless)
lore
Share & tools
lore

Jean-Mickael Guerin

author
#0 lore raw

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

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 <[email protected]>
adm8211/adm8211.c  |    4 +++-
rt2x00/rt2400pci.c |    4 +++-
rt2x00/rt2500pci.c |    4 +++-
rt2x00/rt61pci.c   |    4 +++-
4 files changed, 12 insertions(+), 4 deletions(-)
drivers/net/wireless/d80211/adm8211/adm8211.c +0 -0
index 9fc5da7..53f05c2 100644
@@ -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);
    }
drivers/net/wireless/d80211/rt2x00/rt2400pci.c +3 -1
index 946cf86..1d45851 100644
@@ -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;

}

drivers/net/wireless/d80211/rt2x00/rt2500pci.c +3 -1
index ca0edd5..8d2b3a7 100644
@@ -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;

}

drivers/net/wireless/d80211/rt2x00/rt61pci.c +3 -1
index 0799f9f..47b2eaf 100644
@@ -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;

}

#1 lore raw

Hi,

Quoted 6 lines

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. ;)

drivers/net/wireless/d80211/rt2x00/rt2400pci.c rt2400pci_config_interface(struct net_de · line 1882

Quoted 13 lines
drivers/net/wireless/d80211/rt2x00/rt2400pci.c +3 -1
index 946cf86..1d45851 100644
@@ -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

author
#2 lore raw
Quoted 9 lines

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.

Quoted 3 lines

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...

Quoted 10 lines
Quoted 10 lines
drivers/net/wireless/d80211/rt2x00/rt2400pci.c +0 -0
index 946cf86..1d45851 100644
@@ -1877,7 +1877,9 @@ rt2400pci_config_interface(struct net_de
     if (rt2x00pci->type == IEEE80211_IF_TYPE_MNTR)
         return 0;
Quoted 10 lines

drivers/net/wireless/d80211/rt2x00/rt2400pci.c rt2400pci_config_interface(struct net_de · line 1882

Quoted 4 lines
drivers/net/wireless/d80211/rt2x00/rt2400pci.c +3 -1
-    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.

Quoted 5 lines

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

#3 lore raw
In reply to Jean-Mickael Guerin
Quoted 3 lines · Jean-Mickael Guerin

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

#4 lore raw
Quoted 2 lines · Michael Wu

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.

Quoted 2 lines

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

Signature · 2 lines
Jiri Benc
SUSE Labs
#5 lore raw
In reply to Jean-Mickael Guerin
Quoted 6 lines · Jean-Mickael Guerin

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

Signature · 2 lines
Jiri Benc
SUSE Labs
#6 lore raw

Hi,

Quoted 11 lines
Quoted 6 lines

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.

Quoted 3 lines

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

#7 lore raw

Hi,

Quoted 19 lines
Quoted 11 lines
Quoted 6 lines

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.

Quoted 7 lines
Quoted 3 lines

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