[PATCH] airo: fix setting zero length WEP key

[PATCH] airo: fix setting zero length WEP key

[Stanislaw Gruszka]

Patch prevents call set_wep_key() with zero key length. That fix long
standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
"airo: clean up WEP key operations". Additionally print call trace when
someone will try to use improper parameters, and remove key.len = 0
assignment, because it is in not possible code path.

Reported-and-bisected-by: Chris Siebenmann <cks-rhbugzilla@cs.toronto.edu>
Cc: Dan Williams <dcbw@redhat.com>
Cc: <stable@kernel.org>
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/airo.c |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 4331d67..2a9f029 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,11 +5254,7 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
 	WepKeyRid wkr;
 	int rc;
 
-	if (keylen == 0) {
-		airo_print_err(ai->dev->name, "%s: key length to set was zero",
-			       __func__);
-		return -1;
-	}
+	WARN_ON(keylen == 0);
 
 	memset(&wkr, 0, sizeof(wkr));
 	wkr.len = cpu_to_le16(sizeof(wkr));
@@ -6405,11 +6401,7 @@ static int airo_set_encode(struct net_device *dev,
 		if (dwrq->length > MIN_KEY_SIZE)
 			key.len = MAX_KEY_SIZE;
 		else
-			if (dwrq->length > 0)
-				key.len = MIN_KEY_SIZE;
-			else
-				/* Disable the key */
-				key.len = 0;
+			key.len = MIN_KEY_SIZE;
 		/* Check if the key is not marked as invalid */
 		if(!(dwrq->flags & IW_ENCODE_NOKEY)) {
 			/* Cleanup */
@@ -6590,12 +6582,22 @@ static int airo_set_encodeext(struct net_device *dev,
 		default:
 			return -EINVAL;
 		}
-		/* Send the key to the card */
-		rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
-		if (rc < 0) {
-			airo_print_err(local->dev->name, "failed to set WEP key"
-			               " at index %d: %d.", idx, rc);
-			return rc;
+		if (key.len == 0) {
+			rc = set_wep_tx_idx(local, idx, perm, 1);
+			if (rc < 0) {
+				airo_print_err(local->dev->name,
+					       "failed to set WEP transmit index to %d: %d.",
+					       idx, rc);
+				return rc;
+			}
+		} else {
+			rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
+			if (rc < 0) {
+				airo_print_err(local->dev->name,
+					       "failed to set WEP key at index %d: %d.",
+					       idx, rc);
+				return rc;
+			}
 		}
 	}
 
-- 
1.6.2.5

Re: [PATCH] airo: fix setting zero length WEP key

[Chris Siebenmann]

| Patch prevents call set_wep_key() with zero key length. That fix long
| standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
| "airo: clean up WEP key operations". Additionally print call trace when
| someone will try to use improper parameters, and remove key.len = 0
| assignment, because it is in not possible code path.
| 
| Reported-and-bisected-by: Chris Siebenmann <cks-rhbugzilla@cs.toronto.edu>
| Cc: Dan Williams <dcbw@redhat.com>
| Cc: <stable@kernel.org>
| Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>

Tested-by: Chris Siebenmann <cks@cs.toronto.edu>

(Tested on a Thinkpad T42, using current git tip + this patch; HEAD is
ab658321f32770b903a4426e2a6fae0392757755. This is the same machine I
bisected the problem on.)

	- cks

Re: [PATCH] airo: fix setting zero length WEP key

[Dan Williams]

On Tue, 2010-02-02 at 15:34 +0100, Stanislaw Gruszka wrote:
> Patch prevents call set_wep_key() with zero key length. That fix long
> standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> "airo: clean up WEP key operations". Additionally print call trace when
> someone will try to use improper parameters, and remove key.len = 0
> assignment, because it is in not possible code path.

What problem/regression does this actually fix?  set_wep_key() should
never be called with zero-length key, so we should actually be returning
from that function instead of using WARN_ON() which would allow a
zero-length key to be set (which isn't really valid).

To disable a WEP key, the WEXT caller should be setting
IW_ENCODE_DISABLED in the flags, and *not* sending WEP keys.  Or if they
do for some reason send WEP keys, they need to use the key flag
IW_ENCODE_NOKEY to ensure they key is marked as "invalid" and ignored by
the driver.

Dan

Re: [PATCH] airo: fix setting zero length WEP key

[Chris Siebenmann]

| > Patch prevents call set_wep_key() with zero key
| > length. That fix long standing regression since commit
| > c0380693520b1a1e4f756799a0edc379378b462a "airo: clean up WEP key
| > operations". [...]
|
| What problem/regression does this actually fix? [...]

 That mentioned commit broke wireless on a Thinkpad T42 (running Fedora
11); from that commit onwards, the wireless system appeared to see no
networks. The patch makes things work again.

 More details are in the Fedora bugzilla:
	https://bugzilla.redhat.com/show_bug.cgi?id=522530

	- cks

Re: [PATCH] airo: fix setting zero length WEP key

[Dan Williams]

On Wed, 2010-02-03 at 16:31 -0500, Chris Siebenmann wrote:
> | > Patch prevents call set_wep_key() with zero key
> | > length. That fix long standing regression since commit
> | > c0380693520b1a1e4f756799a0edc379378b462a "airo: clean up WEP key
> | > operations". [...]
> |
> | What problem/regression does this actually fix? [...]
> 
>  That mentioned commit broke wireless on a Thinkpad T42 (running Fedora
> 11); from that commit onwards, the wireless system appeared to see no
> networks. The patch makes things work again.
> 
>  More details are in the Fedora bugzilla:
> 	https://bugzilla.redhat.com/show_bug.cgi?id=522530

You mean with iwlist?  That's quite odd because setting WEP keys doesn't
have anything to do with the scanning stuff, even in the firmware.  Note
that Airo has always been somewhat tempermental for scans and you may or
may not see any or all APs in any given scan.

Dan

Re: [PATCH] airo: fix setting zero length WEP key

[Chris Siebenmann]

| >  That mentioned commit broke wireless on a Thinkpad T42 (running Fedora
| > 11); from that commit onwards, the wireless system appeared to see no
| > networks. The patch makes things work again.
| > 
| >  More details are in the Fedora bugzilla:
| > 	https://bugzilla.redhat.com/show_bug.cgi?id=522530
| 
| You mean with iwlist?  That's quite odd because setting WEP keys
| doesn't have anything to do with the scanning stuff, even in the
| firmware.  Note that Airo has always been somewhat tempermental for
| scans and you may or may not see any or all APs in any given scan.

 All I know is that the failure is total; in all situations I've
tried where a good kernel sees one or more networks and APs, a bad
kernel sees none at all (and 'iwlist wifi0 scanning' reports 'No
scan results'). A good kernel sometimes reports different numbers of
networks and sometimes has quality problems, but a bad kernel never sees
anything.

(Well, has never seen anything within a relatively modest amount of
time.  I have not let it sit for, say, an hour in order to see if
something magically gets better; I have always rebooted into a kernel
with working wireless.)

	- cks

Re: [PATCH] airo: fix setting zero length WEP key

[Dan Williams]

On Thu, 2010-02-04 at 17:10 -0500, Chris Siebenmann wrote:
> | >  That mentioned commit broke wireless on a Thinkpad T42 (running Fedora
> | > 11); from that commit onwards, the wireless system appeared to see no
> | > networks. The patch makes things work again.
> | > 
> | >  More details are in the Fedora bugzilla:
> | > 	https://bugzilla.redhat.com/show_bug.cgi?id=522530
> | 
> | You mean with iwlist?  That's quite odd because setting WEP keys
> | doesn't have anything to do with the scanning stuff, even in the
> | firmware.  Note that Airo has always been somewhat tempermental for
> | scans and you may or may not see any or all APs in any given scan.
> 
>  All I know is that the failure is total; in all situations I've
> tried where a good kernel sees one or more networks and APs, a bad
> kernel sees none at all (and 'iwlist wifi0 scanning' reports 'No
> scan results'). A good kernel sometimes reports different numbers of
> networks and sometimes has quality problems, but a bad kernel never sees
> anything.
> 
> (Well, has never seen anything within a relatively modest amount of
> time.  I have not let it sit for, say, an hour in order to see if
> something magically gets better; I have always rebooted into a kernel
> with working wireless.)

When are you attempting to scan?  Is anything like wpa_supplicant or
NetworkManager running in the background when you do this?

Dan

Re: [PATCH] airo: fix setting zero length WEP key

[Chris Siebenmann]

| When are you attempting to scan?  Is anything like wpa_supplicant or
| NetworkManager running in the background when you do this?

 My primary results are from Fedora 11's GNOME GUI (boot machine, log
in, attempt to connect to one of our wireless networks in various
places); I believe that this uses NetworkManager. I tried with iwlist
now and got the same results, but it looks like wpa_supplicant and
NetworkManager were both running at the time based on pstree output.

	- cks

Re: [PATCH] airo: fix setting zero length WEP key

[Dan Williams]

On Thu, 2010-02-04 at 17:26 -0500, Chris Siebenmann wrote:
> | When are you attempting to scan?  Is anything like wpa_supplicant or
> | NetworkManager running in the background when you do this?
> 
>  My primary results are from Fedora 11's GNOME GUI (boot machine, log
> in, attempt to connect to one of our wireless networks in various
> places); I believe that this uses NetworkManager. I tried with iwlist
> now and got the same results, but it looks like wpa_supplicant and
> NetworkManager were both running at the time based on pstree output.

Ok, that should be fine since wpa_supplicant is the only thing hitting
the airo up for scans and whatnot.  I'm grabbing a kernel now to do some
testing with my 350 (PCMCIA).  I assume you have an internal minicard?

Dan

Re: [PATCH] airo: fix setting zero length WEP key

[Chris Siebenmann]

| > | When are you attempting to scan?  Is anything like wpa_supplicant
| > | or NetworkManager running in the background when you do this?
| >
| >  My primary results are from Fedora 11's GNOME GUI (boot machine,
| > log in, attempt to connect to one of our wireless networks in
| > various places); I believe that this uses NetworkManager. I tried
| > with iwlist now and got the same results, but it looks like
| > wpa_supplicant and NetworkManager were both running at the time
| > based on pstree output.
|
| Ok, that should be fine since wpa_supplicant is the only thing hitting
| the airo up for scans and whatnot.  I'm grabbing a kernel now to do
| some testing with my 350 (PCMCIA).  I assume you have an internal
| minicard?

 I'm not sure what the machine's exact configuration is because I
inherited it (the usual long cycle of passing older and older hardware
around at work), but I believe it must; I certainly don't have anything
plugged into any external slots or sockets.

	- cks

Re: [PATCH] airo: fix setting zero length WEP key

[Dan Williams]

On Thu, 2010-02-04 at 17:42 -0500, Chris Siebenmann wrote:
> | > | When are you attempting to scan?  Is anything like wpa_supplicant
> | > | or NetworkManager running in the background when you do this?
> | >
> | >  My primary results are from Fedora 11's GNOME GUI (boot machine,
> | > log in, attempt to connect to one of our wireless networks in
> | > various places); I believe that this uses NetworkManager. I tried
> | > with iwlist now and got the same results, but it looks like
> | > wpa_supplicant and NetworkManager were both running at the time
> | > based on pstree output.
> |
> | Ok, that should be fine since wpa_supplicant is the only thing hitting
> | the airo up for scans and whatnot.  I'm grabbing a kernel now to do
> | some testing with my 350 (PCMCIA).  I assume you have an internal
> | minicard?
> 
>  I'm not sure what the machine's exact configuration is because I
> inherited it (the usual long cycle of passing older and older hardware
> around at work), but I believe it must; I certainly don't have anything
> plugged into any external slots or sockets.

And just to confirm, what are the exact symptoms that you have with
"broken" kernels?  Just that connections can't be made, the card can't
find access points, etc?  or?

Dan

Re: [PATCH] airo: fix setting zero length WEP key

[Chris Siebenmann]

| >  I'm not sure what the machine's exact configuration is because
| > I inherited it (the usual long cycle of passing older and older
| > hardware around at work), but I believe it must; I certainly don't
| > have anything plugged into any external slots or sockets.
|
| And just to confirm, what are the exact symptoms that you have with
| "broken" kernels?  Just that connections can't be made, the card can't
| find access points, etc? or?

 In the GUI, no wireless networks show up/are listed in the usual
dropdown menu; this means that I can't go as far as even trying to
connect to anything.  From the command line, 'iwlist wifi0 scanning'
lists nothing.

	- cks

Re: [PATCH] airo: fix setting zero length WEP key

[Dan Williams]

On Thu, 2010-02-04 at 18:03 -0500, Chris Siebenmann wrote:
> | >  I'm not sure what the machine's exact configuration is because
> | > I inherited it (the usual long cycle of passing older and older
> | > hardware around at work), but I believe it must; I certainly don't
> | > have anything plugged into any external slots or sockets.
> |
> | And just to confirm, what are the exact symptoms that you have with
> | "broken" kernels?  Just that connections can't be made, the card can't
> | find access points, etc? or?
> 
>  In the GUI, no wireless networks show up/are listed in the usual
> dropdown menu; this means that I can't go as far as even trying to
> connect to anything.  From the command line, 'iwlist wifi0 scanning'
> lists nothing.

Wait, wifi0?  You need to be using 'eth0' or 'eth1', whichever eth
device that 'iwconfig' reports.  wifi0 is a historical airo anomaly
that's only used for packet capture and sniffing of the 802.11 frames.
It should not be used for anything other than Wireshark basically.

Do you get the same behavior if you use the normal ethX interface of the
airo device?

Dan

Re: [PATCH] airo: fix setting zero length WEP key

[Dan Williams]

On Thu, 2010-02-04 at 15:26 -0800, Dan Williams wrote:
> On Thu, 2010-02-04 at 18:03 -0500, Chris Siebenmann wrote:
> > | >  I'm not sure what the machine's exact configuration is because
> > | > I inherited it (the usual long cycle of passing older and older
> > | > hardware around at work), but I believe it must; I certainly don't
> > | > have anything plugged into any external slots or sockets.
> > |
> > | And just to confirm, what are the exact symptoms that you have with
> > | "broken" kernels?  Just that connections can't be made, the card can't
> > | find access points, etc? or?
> > 
> >  In the GUI, no wireless networks show up/are listed in the usual
> > dropdown menu; this means that I can't go as far as even trying to
> > connect to anything.  From the command line, 'iwlist wifi0 scanning'
> > lists nothing.
> 
> Wait, wifi0?  You need to be using 'eth0' or 'eth1', whichever eth
> device that 'iwconfig' reports.  wifi0 is a historical airo anomaly
> that's only used for packet capture and sniffing of the 802.11 frames.
> It should not be used for anything other than Wireshark basically.
> 
> Do you get the same behavior if you use the normal ethX interface of the
> airo device?

Note that just because wifi0 may return scan results doesn't mean it
should be used with iwconfig...

But also, you're doing the iwlist scan as root, correct?

Dan

Re: [PATCH] airo: fix setting zero length WEP key

[Chris Siebenmann]

| >  In the GUI, no wireless networks show up/are listed in the usual
| > dropdown menu; this means that I can't go as far as even trying to
| > connect to anything.  From the command line, 'iwlist wifi0 scanning'
| > lists nothing.
|
| Wait, wifi0?  You need to be using 'eth0' or 'eth1', whichever eth
| device that 'iwconfig' reports. wifi0 is a historical airo anomaly
| that's only used for packet capture and sniffing of the 802.11 frames.
| It should not be used for anything other than Wireshark basically.
|
| Do you get the same behavior if you use the normal ethX interface of
| the airo device?

 Yes; on a 'bad' kernel, 'iwlist eth1 scanning' lists nothing, and on a
good kernel it lists the networks that I expect (with some variability;
where I have the machine at the moment is on the edge of visibility for
some networks, and they appear and disappear periodically).

| Note that just because wifi0 may return scan results doesn't mean it
| should be used with iwconfig...
|
| But also, you're doing the iwlist scan as root, correct?

 Yes (on both the good and the bad kernels).

 I haven't been trying to use iwconfig for anything; all of my actual
use of the wireless networks has been done through the Fedora 11 GUI.

	- cks

Re: [PATCH] airo: fix setting zero length WEP key

[Dan Williams]

On Thu, 2010-02-04 at 18:36 -0500, Chris Siebenmann wrote:
> | >  In the GUI, no wireless networks show up/are listed in the usual
> | > dropdown menu; this means that I can't go as far as even trying to
> | > connect to anything.  From the command line, 'iwlist wifi0 scanning'
> | > lists nothing.
> |
> | Wait, wifi0?  You need to be using 'eth0' or 'eth1', whichever eth
> | device that 'iwconfig' reports. wifi0 is a historical airo anomaly
> | that's only used for packet capture and sniffing of the 802.11 frames.
> | It should not be used for anything other than Wireshark basically.
> |
> | Do you get the same behavior if you use the normal ethX interface of
> | the airo device?
> 
>  Yes; on a 'bad' kernel, 'iwlist eth1 scanning' lists nothing, and on a
> good kernel it lists the networks that I expect (with some variability;
> where I have the machine at the moment is on the edge of visibility for
> some networks, and they appear and disappear periodically).
> 
> | Note that just because wifi0 may return scan results doesn't mean it
> | should be used with iwconfig...
> |
> | But also, you're doing the iwlist scan as root, correct?
> 
>  Yes (on both the good and the bad kernels).
> 
>  I haven't been trying to use iwconfig for anything; all of my actual
> use of the wireless networks has been done through the Fedora 11 GUI.

I've been able to reproduce the issue with my 350.  Note that even with
the original patch, the 350 will never return a scan result *other* than
the associated AP.  If you turn off the AP, the airo will not return any
scan results even though before it connected to the AP it would return a
bunch.

Airo firmware is, well, old and crappy, and nobody ever got enough
documentation out of Cisco to get the driver working extremely well.

But oddly enough, the patch does fix the issue, though I have a slightly
modified version that I'd rather be applied (see other mails).

Dan

Re: [PATCH] airo: fix setting zero length WEP key

[Stanislaw Gruszka]

On Wed, Feb 03, 2010 at 12:43:44PM -0800, Dan Williams wrote:
> On Tue, 2010-02-02 at 15:34 +0100, Stanislaw Gruszka wrote:
> > Patch prevents call set_wep_key() with zero key length. That fix long
> > standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> > "airo: clean up WEP key operations". Additionally print call trace when
> > someone will try to use improper parameters, and remove key.len = 0
> > assignment, because it is in not possible code path.
> 
> What problem/regression does this actually fix?  set_wep_key() should
> never be called with zero-length key, 
There was one case where set_wep_key() was called instead of
set_wep_tx_idx().

> so we should actually be returning
> from that function instead of using WARN_ON() which would allow a
> zero-length key to be set (which isn't really valid).
Ok, changing that in v2 patch.

Cheers
Stanislaw

[PATCH v2] airo: fix setting zero length WEP key

[Stanislaw Gruszka]

Patch prevents call set_wep_key() with zero key length. That fix long
standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
"airo: clean up WEP key operations". Additionally print call trace when
someone will try to use improper parameters, and remove key.len = 0
assignment, because it is in not possible code path.

v1->v2
Return instantly from set_wep_key() when keylen == 0.

Reported-and-bisected-by: Chris Siebenmann <cks-rhbugzilla@cs.toronto.edu>
Cc: Dan Williams <dcbw@redhat.com>
Cc: <stable@kernel.org>
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/airo.c |   33 ++++++++++++++++++---------------
 1 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 4331d67..01bffc2 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,11 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
 	WepKeyRid wkr;
 	int rc;
 
-	if (keylen == 0) {
-		airo_print_err(ai->dev->name, "%s: key length to set was zero",
-			       __func__);
+	if (WARN_ON(keylen == 0))
 		return -1;
-	}
 
 	memset(&wkr, 0, sizeof(wkr));
 	wkr.len = cpu_to_le16(sizeof(wkr));
@@ -6405,11 +6402,7 @@ static int airo_set_encode(struct net_device *dev,
 		if (dwrq->length > MIN_KEY_SIZE)
 			key.len = MAX_KEY_SIZE;
 		else
-			if (dwrq->length > 0)
-				key.len = MIN_KEY_SIZE;
-			else
-				/* Disable the key */
-				key.len = 0;
+			key.len = MIN_KEY_SIZE;
 		/* Check if the key is not marked as invalid */
 		if(!(dwrq->flags & IW_ENCODE_NOKEY)) {
 			/* Cleanup */
@@ -6590,12 +6583,22 @@ static int airo_set_encodeext(struct net_device *dev,
 		default:
 			return -EINVAL;
 		}
-		/* Send the key to the card */
-		rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
-		if (rc < 0) {
-			airo_print_err(local->dev->name, "failed to set WEP key"
-			               " at index %d: %d.", idx, rc);
-			return rc;
+		if (key.len == 0) {
+			rc = set_wep_tx_idx(local, idx, perm, 1);
+			if (rc < 0) {
+				airo_print_err(local->dev->name,
+					       "failed to set WEP transmit index to %d: %d.",
+					       idx, rc);
+				return rc;
+			}
+		} else {
+			rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
+			if (rc < 0) {
+				airo_print_err(local->dev->name,
+					       "failed to set WEP key at index %d: %d.",
+					       idx, rc);
+				return rc;
+			}
 		}
 	}
 
-- 
1.6.2.5

Re: [PATCH v2] airo: fix setting zero length WEP key

[Chris Siebenmann]

| Patch prevents call set_wep_key() with zero key length. That fix long
| standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
| "airo: clean up WEP key operations". Additionally print call trace when
| someone will try to use improper parameters, and remove key.len = 0
| assignment, because it is in not possible code path.
| 
| v1->v2
| Return instantly from set_wep_key() when keylen == 0.
| 
| Reported-and-bisected-by: Chris Siebenmann <cks-rhbugzilla@cs.toronto.edu>
| Cc: Dan Williams <dcbw@redhat.com>
| Cc: <stable@kernel.org>
| Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>

Tested-by: Chris Siebenmann <cks@cs.toronto.edu>

(Tested on a Thinkpad T42, using current git tip + this patch; HEAD is
ab658321f32770b903a4426e2a6fae0392757755. This is the same machine I
bisected the problem on.)

	- cks

Re: [PATCH v2] airo: fix setting zero length WEP key

[Dan Williams]

On Thu, 2010-02-04 at 13:07 +0100, Stanislaw Gruszka wrote:
> Patch prevents call set_wep_key() with zero key length. That fix long
> standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> "airo: clean up WEP key operations". Additionally print call trace when
> someone will try to use improper parameters, and remove key.len = 0
> assignment, because it is in not possible code path.

Could you send the hunk that touches airo_set_encode() separately
please?  That's obviously correct and an easy ack, but isn't really
related to the other hunks here.

Thanks,
Dan

> v1->v2
> Return instantly from set_wep_key() when keylen == 0.
> 
> Reported-and-bisected-by: Chris Siebenmann <cks-rhbugzilla@cs.toronto.edu>
> Cc: Dan Williams <dcbw@redhat.com>
> Cc: <stable@kernel.org>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/airo.c |   33 ++++++++++++++++++---------------
>  1 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
> index 4331d67..01bffc2 100644
> --- a/drivers/net/wireless/airo.c
> +++ b/drivers/net/wireless/airo.c
> @@ -5254,11 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
>  	WepKeyRid wkr;
>  	int rc;
>  
> -	if (keylen == 0) {
> -		airo_print_err(ai->dev->name, "%s: key length to set was zero",
> -			       __func__);
> +	if (WARN_ON(keylen == 0))
>  		return -1;
> -	}
>  
>  	memset(&wkr, 0, sizeof(wkr));
>  	wkr.len = cpu_to_le16(sizeof(wkr));
> @@ -6405,11 +6402,7 @@ static int airo_set_encode(struct net_device *dev,
>  		if (dwrq->length > MIN_KEY_SIZE)
>  			key.len = MAX_KEY_SIZE;
>  		else
> -			if (dwrq->length > 0)
> -				key.len = MIN_KEY_SIZE;
> -			else
> -				/* Disable the key */
> -				key.len = 0;
> +			key.len = MIN_KEY_SIZE;
>  		/* Check if the key is not marked as invalid */
>  		if(!(dwrq->flags & IW_ENCODE_NOKEY)) {
>  			/* Cleanup */
> @@ -6590,12 +6583,22 @@ static int airo_set_encodeext(struct net_device *dev,
>  		default:
>  			return -EINVAL;
>  		}
> -		/* Send the key to the card */
> -		rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
> -		if (rc < 0) {
> -			airo_print_err(local->dev->name, "failed to set WEP key"
> -			               " at index %d: %d.", idx, rc);
> -			return rc;
> +		if (key.len == 0) {
> +			rc = set_wep_tx_idx(local, idx, perm, 1);
> +			if (rc < 0) {
> +				airo_print_err(local->dev->name,
> +					       "failed to set WEP transmit index to %d: %d.",
> +					       idx, rc);
> +				return rc;
> +			}
> +		} else {
> +			rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
> +			if (rc < 0) {
> +				airo_print_err(local->dev->name,
> +					       "failed to set WEP key at index %d: %d.",
> +					       idx, rc);
> +				return rc;
> +			}
>  		}
>  	}
>  

Re: [PATCH v2] airo: fix setting zero length WEP key

[Stanislaw Gruszka]

On Thu, Feb 04, 2010 at 03:56:13PM -0800, Dan Williams wrote:
> On Thu, 2010-02-04 at 13:07 +0100, Stanislaw Gruszka wrote:
> > Patch prevents call set_wep_key() with zero key length. That fix long
> > standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> > "airo: clean up WEP key operations". Additionally print call trace when
> > someone will try to use improper parameters, and remove key.len = 0
> > assignment, because it is in not possible code path.
> 
> Could you send the hunk that touches airo_set_encode() separately
> please?  That's obviously correct and an easy ack, but isn't really
> related to the other hunks here.

I'm confused by your further patch, I'm deferring fixing this to you :)

Stanislaw

Re: [PATCH v2] airo: fix setting zero length WEP key

[Dan Williams]

John, can we revert the patch you applied, and apply the following patch
instead (in the next mail)?  I'd rather use the one I'm about to send
because it makes it clearer *why* setting the TX key index is required,
and it limits that case to only the time when it's necessary.  Pending
the testing by Chris, I'd rather have the revised version committed
instead.

Thanks,
Dan

[PATCH] airo: fix WEP key clearing after c0380693520b1a1e4f756799a0edc379378b462a

[Dan Williams]

The firmware apparently needs the WEP key RID touched when turning WEP
off, otherwise it gets angry and refuses to return scan results.  The
firmware doesn't handle zero-length WEP keys so quite the warning
message that was inadvertently printed when disabling WEP, which
wpa_supplicant could trigger repeatedly.

Signed-off-by: Dan Williams <dcbw@redhat.com>

---

diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 4331d67..38902c7 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,11 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
 	WepKeyRid wkr;
 	int rc;
 
-	if (keylen == 0) {
-		airo_print_err(ai->dev->name, "%s: key length to set was zero",
-			       __func__);
+	if (WARN_ON (keylen == 0))
 		return -1;
-	}
 
 	memset(&wkr, 0, sizeof(wkr));
 	wkr.len = cpu_to_le16(sizeof(wkr));
@@ -6532,7 +6529,7 @@ static int airo_set_encodeext(struct net_device *dev,
 	struct iw_encode_ext *ext = (struct iw_encode_ext *)extra;
 	int perm = ( encoding->flags & IW_ENCODE_TEMP ? 0 : 1 );
 	__le16 currentAuthType = local->config.authType;
-	int idx, key_len, alg = ext->alg, set_key = 1, rc;
+	int idx, key_len, alg = ext->alg, rc;
 	wep_key_t key;
 
 	if (!local->wep_capable)
@@ -6566,10 +6563,9 @@ static int airo_set_encodeext(struct net_device *dev,
 			               idx, rc);
 			return rc;
 		}
-		set_key = ext->key_len > 0 ? 1 : 0;
 	}
 
-	if (set_key) {
+	if (ext->key_len > 0) {
 		/* Set the requested key first */
 		memset(key.key, 0, MAX_KEY_SIZE);
 		switch (alg) {
@@ -6600,12 +6596,20 @@ static int airo_set_encodeext(struct net_device *dev,
 	}
 
 	/* Read the flags */
-	if(encoding->flags & IW_ENCODE_DISABLED)
+	if (encoding->flags & IW_ENCODE_DISABLED) {
+		/* The firmware seems to need the WEP key RID touched when
+		 * setting WEP disabled; resetting the transmit key index to 0
+		 * is good enough.  Otherwise it gets confused and stops
+		 * delivering scan results (!).
+		 */
+		if (!(ext->ext_flags & IW_ENCODE_EXT_SET_TX_KEY))
+			set_wep_tx_idx(local, 0, 0, 1);
 		local->config.authType = AUTH_OPEN;	// disable encryption
-	if(encoding->flags & IW_ENCODE_RESTRICTED)
+	} else if (encoding->flags & IW_ENCODE_RESTRICTED)
 		local->config.authType = AUTH_SHAREDKEY;	// Only Both
-	if(encoding->flags & IW_ENCODE_OPEN)
+	else if (encoding->flags & IW_ENCODE_OPEN)
 		local->config.authType = AUTH_ENCRYPT;	// Only Wep
+
 	/* Commit the changes to flags if needed */
 	if (local->config.authType != currentAuthType)
 		set_bit (FLAG_COMMIT, &local->flags);

Re: [PATCH] airo: fix WEP key clearing after c0380693520b1a1e4f756799a0edc379378b462a

[Chris Siebenmann]

| The firmware apparently needs the WEP key RID touched when turning
| WEP off, otherwise it gets angry and refuses to return scan results.
| The firmware doesn't handle zero-length WEP keys so quite the warning
| message that was inadvertently printed when disabling WEP, which
| wpa_supplicant could trigger repeatedly.
|
| Signed-off-by: Dan Williams <dcbw@redhat.com>

 This patch does *not* work for me; with it applied to the 2.6.33-rc6
from-git that I have been using as the testing basis, the resulting
kernel does not see any wireless networks. dmesg has multiple reports
of:

	airo(eth1): cmd:103 status:7f03 rsp0:0 rsp1:ff10 rsp2:c0f0
	airo(eth1): cmd:103 status:7f03 rsp0:0 rsp1:ff10 rsp2:c0f0

	- cks

Re: [PATCH] airo: fix WEP key clearing after c0380693520b1a1e4f756799a0edc379378b462a

[Dan Williams]

On Fri, 2010-02-05 at 11:26 -0500, Chris Siebenmann wrote:
> | The firmware apparently needs the WEP key RID touched when turning
> | WEP off, otherwise it gets angry and refuses to return scan results.
> | The firmware doesn't handle zero-length WEP keys so quite the warning
> | message that was inadvertently printed when disabling WEP, which
> | wpa_supplicant could trigger repeatedly.
> |
> | Signed-off-by: Dan Williams <dcbw@redhat.com>
> 
>  This patch does *not* work for me; with it applied to the 2.6.33-rc6
> from-git that I have been using as the testing basis, the resulting
> kernel does not see any wireless networks. dmesg has multiple reports

Yes, I know it may have worked in earlier kernels.  And we should find
out why that is, and hopefully fix the regression if any.  But
understand that airo has always been problematic and has never worked as
well as other fullmac cards like orinoco.

I booted up RHEL5 today, which uses a 2.6.18 kernel and of course does
not have the patch-in-question.  That behavior is:

1) stop NetworkManager and wpa_supplicant
2) insert airo card
3) iwlist scan returns many results
4) connect to WEP-enabled AP
5) iwlist scan returns *only* the connected ap
6) iwconfig eth1 essid any key off
7) iwlist scan returns no results

remember, that's without any of the patches we're talking about, using a
kernel from 2007.  Which is about the same behavior you get, correct?

> of:
> 
> 	airo(eth1): cmd:103 status:7f03 rsp0:0 rsp1:ff10 rsp2:c0f0
> 	airo(eth1): cmd:103 status:7f03 rsp0:0 rsp1:ff10 rsp2:c0f0

CMD 0x0103 is the LISTBSS command, which is sent to trigger a scan.
What firmware version do you have?  It should get printed out in 'dmesg'
when loading the driver.

Dan

Re: [PATCH] airo: fix WEP key clearing after c0380693520b1a1e4f756799a0edc379378b462a

[Chris Siebenmann]

| Yes, I know it may have worked in earlier kernels.  And we should find
| out why that is, and hopefully fix the regression if any.  But
| understand that airo has always been problematic and has never worked as
| well as other fullmac cards like orinoco.
| 
| I booted up RHEL5 today, which uses a 2.6.18 kernel and of course does
| not have the patch-in-question.  That behavior is:
| 
| 1) stop NetworkManager and wpa_supplicant
| 2) insert airo card
| 3) iwlist scan returns many results
| 4) connect to WEP-enabled AP
| 5) iwlist scan returns *only* the connected ap
| 6) iwconfig eth1 essid any key off
| 7) iwlist scan returns no results
| 
| remember, that's without any of the patches we're talking about, using a
| kernel from 2007.  Which is about the same behavior you get, correct?

 I don't get quite this behavior, although I'm not doing exactly these
steps; instead, I do:
	- boot machine
	- use 'iwlist scan' and see multiple APs
	- use NetworkManager to connect to our WEP-enabled AP
	- 'iwlist scan' returns only the single AP
	- turn things off with:
		iwconfig eth1 essid ANY key off
	- 'iwconfig eth1' and NetworkManager both show no connected AP
	- 'iwlist scan' returns only the ex-connected AP

(I do not know the right commands to connect to our WEP-enabled AP and
verify that everything is working right without using NetworkManager,
so I used it for the connection phase.)

	- cks

Re: [PATCH] airo: fix WEP key clearing after c0380693520b1a1e4f756799a0edc379378b462a

[Dan Williams]

On Fri, 2010-02-05 at 18:12 -0500, Chris Siebenmann wrote:
> | Yes, I know it may have worked in earlier kernels.  And we should find
> | out why that is, and hopefully fix the regression if any.  But
> | understand that airo has always been problematic and has never worked as
> | well as other fullmac cards like orinoco.
> | 
> | I booted up RHEL5 today, which uses a 2.6.18 kernel and of course does
> | not have the patch-in-question.  That behavior is:
> | 
> | 1) stop NetworkManager and wpa_supplicant
> | 2) insert airo card
> | 3) iwlist scan returns many results
> | 4) connect to WEP-enabled AP
> | 5) iwlist scan returns *only* the connected ap
> | 6) iwconfig eth1 essid any key off
> | 7) iwlist scan returns no results
> | 
> | remember, that's without any of the patches we're talking about, using a
> | kernel from 2007.  Which is about the same behavior you get, correct?
> 
>  I don't get quite this behavior, although I'm not doing exactly these
> steps; instead, I do:
> 	- boot machine
> 	- use 'iwlist scan' and see multiple APs
> 	- use NetworkManager to connect to our WEP-enabled AP
> 	- 'iwlist scan' returns only the single AP
> 	- turn things off with:
> 		iwconfig eth1 essid ANY key off
> 	- 'iwconfig eth1' and NetworkManager both show no connected AP
> 	- 'iwlist scan' returns only the ex-connected AP

These are the results you get on the older kernel without the commit
we're all talking about here?

> (I do not know the right commands to connect to our WEP-enabled AP and
> verify that everything is working right without using NetworkManager,
> so I used it for the connection phase.)

iwconfig ethX essid 'foobar' key <your hex wep key>

Then run 'iwconfig ethX' over and over until you see a valid AP BSSID
listed in the result, and then you can run 'dhclient -1 -v -d ethX' to
try and DHCP.

Also, what firmware version do you have?

Dan

Re: [PATCH] airo: fix WEP key clearing after c0380693520b1a1e4f756799a0edc379378b462a

[Chris Siebenmann]

 So that I don't forget this again:
| Also, what firmware version do you have?

 The driver reports:
airo(): Probing for PCI adapters
airo 0000:02:02.0: PCI INT A -> Link[LNKC] -> GSI 11 (level, low) -> IRQ 11
airo(): Found an MPI350 card
airo(eth1): Firmware version 5.41.00
airo(eth1): WPA supported.
airo(eth1): MAC enabled 00:0e:9b:90:af:29

	- cks

Re: [PATCH] airo: fix WEP key clearing after c0380693520b1a1e4f756799a0edc379378b462a

[Chris Siebenmann]

| >  I don't get quite this behavior, although I'm not doing exactly these
| > steps; instead, I do:
| > 	- boot machine
| > 	- use 'iwlist scan' and see multiple APs
| > 	- use NetworkManager to connect to our WEP-enabled AP
| > 	- 'iwlist scan' returns only the single AP
| > 	- turn things off with:
| > 		iwconfig eth1 essid ANY key off
| > 	- 'iwconfig eth1' and NetworkManager both show no connected AP
| > 	- 'iwlist scan' returns only the ex-connected AP
| 
| These are the results you get on the older kernel without the commit
| we're all talking about here?

 I have been testing on the kernel with working wireless,
specifically Fedora 11's 2.6.29.6-217.2.8.fc11 kernel.

 I have to amend my testing results, however. I have now managed to get
the network live with iwconfig, not NetworkManager, and when I do this
and do 'iwconfig eth1 essid ANY key off', I actually see *multiple*
APs with 'iwlist scan' if I catch it at the right time. (In fact, I
sometimes associate with an unprotected network in range.)

(There is only one good network in range, which is ours, but I'm on
the edge of a number that sometimes show up in normal scan output and
sometimes don't.)

 So this doesn't match the behavior you were seeing with your old kernel
and machine.

	- cks

Re: [PATCH v2] airo: fix setting zero length WEP key

[John W. Linville]

On Thu, Feb 04, 2010 at 10:59:38PM -0800, Dan Williams wrote:
> John, can we revert the patch you applied, and apply the following patch
> instead (in the next mail)?  I'd rather use the one I'm about to send
> because it makes it clearer *why* setting the TX key index is required,
> and it limits that case to only the time when it's necessary.  Pending
> the testing by Chris, I'd rather have the revised version committed
> instead.

Do you still want this don, even after Chris's testing says your
patch doesn't work?  I wasn't sure from your response.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH v2] airo: fix setting zero length WEP key

[Dan Williams]

On Mon, 2010-02-08 at 15:55 -0500, John W. Linville wrote:
> On Thu, Feb 04, 2010 at 10:59:38PM -0800, Dan Williams wrote:
> > John, can we revert the patch you applied, and apply the following patch
> > instead (in the next mail)?  I'd rather use the one I'm about to send
> > because it makes it clearer *why* setting the TX key index is required,
> > and it limits that case to only the time when it's necessary.  Pending
> > the testing by Chris, I'd rather have the revised version committed
> > instead.
> 
> Do you still want this don, even after Chris's testing says your
> patch doesn't work?  I wasn't sure from your response.

Not yet, no.  I'd like to work out the issue first, if we can.

Thanks,
Dan

Re: [PATCH v2] airo: fix setting zero length WEP key

[John W. Linville]

On Thu, Feb 04, 2010 at 01:07:13PM +0100, Stanislaw Gruszka wrote:
> Patch prevents call set_wep_key() with zero key length. That fix long
> standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> "airo: clean up WEP key operations". Additionally print call trace when
> someone will try to use improper parameters, and remove key.len = 0
> assignment, because it is in not possible code path.
> 
> v1->v2
> Return instantly from set_wep_key() when keylen == 0.
> 
> Reported-and-bisected-by: Chris Siebenmann <cks-rhbugzilla@cs.toronto.edu>
> Cc: Dan Williams <dcbw@redhat.com>
> Cc: <stable@kernel.org>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>

Stanislaw, your original patch was already committed in
wireless-next-2.6.  Could you send a second patch with just your
v1->v2 change?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH] airo: return from set_wep_key() when key length is zero

[Stanislaw Gruszka]

Even if keylen == 0 is a bug and should not really happen, better avoid
possibility of passing bad value to firmware.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/airo.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 2a9f029..01bffc2 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,7 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
 	WepKeyRid wkr;
 	int rc;
 
-	WARN_ON(keylen == 0);
+	if (WARN_ON(keylen == 0))
+		return -1;
 
 	memset(&wkr, 0, sizeof(wkr));
 	wkr.len = cpu_to_le16(sizeof(wkr));
-- 
1.6.2.5

[PATCH resend] airo: return from set_wep_key() when key length is zero

[Stanislaw Gruszka]

Even if keylen == 0 is a bug and should not really happen, better avoid
possibility of passing bad value to firmware.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/airo.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 2a9f029..01bffc2 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,7 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
 	WepKeyRid wkr;
 	int rc;
 
-	WARN_ON(keylen == 0);
+	if (WARN_ON(keylen == 0))
+		return -1;
 
 	memset(&wkr, 0, sizeof(wkr));
 	wkr.len = cpu_to_le16(sizeof(wkr));
-- 
1.6.2.5

Re: [PATCH resend] airo: return from set_wep_key() when key length is zero

[Dan Williams]

On Fri, 2010-02-26 at 15:10 +0100, Stanislaw Gruszka wrote:
> Even if keylen == 0 is a bug and should not really happen, better avoid
> possibility of passing bad value to firmware.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>

I actually told John to just apply your first patch via IRC, but perhaps
he hasn't done that yet.  I cannot reproduce the issue that Chris
encountered with my PCMCIA 350 using 5.60 firmware, and I bricked my
card loading 5.40 firmware (same as Chris) onto it.

I couldn't find any failures your original patch on my 5.60 device, my
objections were simply code flow and semantics.  So I'll ack the
original patch.

Dan


> ---
>  drivers/net/wireless/airo.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
> index 2a9f029..01bffc2 100644
> --- a/drivers/net/wireless/airo.c
> +++ b/drivers/net/wireless/airo.c
> @@ -5254,7 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
>  	WepKeyRid wkr;
>  	int rc;
>  
> -	WARN_ON(keylen == 0);
> +	if (WARN_ON(keylen == 0))
> +		return -1;
>  
>  	memset(&wkr, 0, sizeof(wkr));
>  	wkr.len = cpu_to_le16(sizeof(wkr));

Re: [PATCH resend] airo: return from set_wep_key() when key length is zero

[John W. Linville]

On Fri, Feb 26, 2010 at 03:09:29PM -0800, Dan Williams wrote:
> On Fri, 2010-02-26 at 15:10 +0100, Stanislaw Gruszka wrote:
> > Even if keylen == 0 is a bug and should not really happen, better avoid
> > possibility of passing bad value to firmware.
> > 
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> 
> I actually told John to just apply your first patch via IRC, but perhaps
> he hasn't done that yet.  I cannot reproduce the issue that Chris
> encountered with my PCMCIA 350 using 5.60 firmware, and I bricked my
> card loading 5.40 firmware (same as Chris) onto it.
> 
> I couldn't find any failures your original patch on my 5.60 device, my
> objections were simply code flow and semantics.  So I'll ack the
> original patch.

commit f09c256375c7cf1e112b8ef6306cdd313490d7c0
Author: Stanislaw Gruszka <sgruszka@redhat.com>
Date:   Tue Feb 2 15:34:50 2010 +0100

    airo: fix setting zero length WEP key
    
    Patch prevents call set_wep_key() with zero key length. That fix long
    standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
    "airo: clean up WEP key operations". Additionally print call trace when
    someone will try to use improper parameters, and remove key.len = 0
    assignment, because it is in not possible code path.
    
    Reported-by: Chris Siebenmann <cks-rhbugzilla@cs.toronto.edu>
    Bisected-by: Chris Siebenmann <cks-rhbugzilla@cs.toronto.edu>
    Tested-by: Chris Siebenmann <cks@cs.toronto.edu>
    Cc: Dan Williams <dcbw@redhat.com>
    Cc: <stable@kernel.org>
    Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>

Hth...

John
-- 
John W. Linville                Someday the world will need a hero, and you
linville@tuxdriver.com                  might be all we have.  Be ready.

Re: [PATCH resend] airo: return from set_wep_key() when key length is zero

[Stanislaw Gruszka]

On Fri, Feb 26, 2010 at 08:26:17PM -0500, John W. Linville wrote:
> On Fri, Feb 26, 2010 at 03:09:29PM -0800, Dan Williams wrote:
> > On Fri, 2010-02-26 at 15:10 +0100, Stanislaw Gruszka wrote:
> > > Even if keylen == 0 is a bug and should not really happen, better avoid
> > > possibility of passing bad value to firmware.
> > > 
> > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>

This is small v1->v2 patch on top of below commit. You asked for it :)

> commit f09c256375c7cf1e112b8ef6306cdd313490d7c0
> Author: Stanislaw Gruszka <sgruszka@redhat.com>
> Date:   Tue Feb 2 15:34:50 2010 +0100
> 
>     airo: fix setting zero length WEP key

Stanislaw

Re: [PATCH v2] airo: fix setting zero length WEP key

[Dan Williams]

On Thu, 2010-02-04 at 13:07 +0100, Stanislaw Gruszka wrote:
> Patch prevents call set_wep_key() with zero key length. That fix long
> standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> "airo: clean up WEP key operations". Additionally print call trace when
> someone will try to use improper parameters, and remove key.len = 0
> assignment, because it is in not possible code path.
> 
> v1->v2
> Return instantly from set_wep_key() when keylen == 0.
> 
> Reported-and-bisected-by: Chris Siebenmann <cks-rhbugzilla@cs.toronto.edu>
> Cc: Dan Williams <dcbw@redhat.com>
> Cc: <stable@kernel.org>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>

Acked-by: Dan Williams <dcbw@redhat.com>

> ---
>  drivers/net/wireless/airo.c |   33 ++++++++++++++++++---------------
>  1 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
> index 4331d67..01bffc2 100644
> --- a/drivers/net/wireless/airo.c
> +++ b/drivers/net/wireless/airo.c
> @@ -5254,11 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
>  	WepKeyRid wkr;
>  	int rc;
>  
> -	if (keylen == 0) {
> -		airo_print_err(ai->dev->name, "%s: key length to set was zero",
> -			       __func__);
> +	if (WARN_ON(keylen == 0))
>  		return -1;
> -	}
>  
>  	memset(&wkr, 0, sizeof(wkr));
>  	wkr.len = cpu_to_le16(sizeof(wkr));
> @@ -6405,11 +6402,7 @@ static int airo_set_encode(struct net_device *dev,
>  		if (dwrq->length > MIN_KEY_SIZE)
>  			key.len = MAX_KEY_SIZE;
>  		else
> -			if (dwrq->length > 0)
> -				key.len = MIN_KEY_SIZE;
> -			else
> -				/* Disable the key */
> -				key.len = 0;
> +			key.len = MIN_KEY_SIZE;
>  		/* Check if the key is not marked as invalid */
>  		if(!(dwrq->flags & IW_ENCODE_NOKEY)) {
>  			/* Cleanup */
> @@ -6590,12 +6583,22 @@ static int airo_set_encodeext(struct net_device *dev,
>  		default:
>  			return -EINVAL;
>  		}
> -		/* Send the key to the card */
> -		rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
> -		if (rc < 0) {
> -			airo_print_err(local->dev->name, "failed to set WEP key"
> -			               " at index %d: %d.", idx, rc);
> -			return rc;
> +		if (key.len == 0) {
> +			rc = set_wep_tx_idx(local, idx, perm, 1);
> +			if (rc < 0) {
> +				airo_print_err(local->dev->name,
> +					       "failed to set WEP transmit index to %d: %d.",
> +					       idx, rc);
> +				return rc;
> +			}
> +		} else {
> +			rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
> +			if (rc < 0) {
> +				airo_print_err(local->dev->name,
> +					       "failed to set WEP key at index %d: %d.",
> +					       idx, rc);
> +				return rc;
> +			}
>  		}
>  	}
>