[PATCH 2.6.40] wl12xx: fix oops in sched_scan when forcing a passive scan

[PATCH 2.6.40] wl12xx: fix oops in sched_scan when forcing a passive scan

[Luciano Coelho]

Fix kernel oops when trying to use passive scheduled scans.  The
reason was that in passive scans there are no SSIDs, so there was a
NULL pointer dereference.

To solve the problem, we now check the number of SSIDs provided in the
sched_scan request and only access the list if there's one or more
(ie. passive scan is not forced).  We also move the channels from
active to passive if passive scanning is forced.  For this to work,
it's necessary to set both active and passive dwell times for all
channels.

Signed-off-by: Luciano Coelho <coelho@ti.com>
---
 drivers/net/wireless/wl12xx/scan.c |   31 +++++++++++++++++++------------
 1 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/scan.c b/drivers/net/wireless/wl12xx/scan.c
index f37e5a3..d379aac 100644
--- a/drivers/net/wireless/wl12xx/scan.c
+++ b/drivers/net/wireless/wl12xx/scan.c
@@ -350,15 +350,12 @@ wl1271_scan_get_sched_scan_channels(struct wl1271 *wl,
 			wl1271_debug(DEBUG_SCAN, "max_power %d",
 				     req->channels[i]->max_power);
 
-			if (flags & IEEE80211_CHAN_PASSIVE_SCAN) {
-				channels[j].passive_duration =
-					cpu_to_le16(c->dwell_time_passive);
-			} else {
-				channels[j].min_duration =
-					cpu_to_le16(c->min_dwell_time_active);
-				channels[j].max_duration =
-					cpu_to_le16(c->max_dwell_time_active);
-			}
+			channels[j].passive_duration =
+				cpu_to_le16(c->dwell_time_passive);
+			channels[j].min_duration =
+				cpu_to_le16(c->min_dwell_time_active);
+			channels[j].max_duration =
+				cpu_to_le16(c->max_dwell_time_active);
 			channels[j].tx_power_att = req->channels[j]->max_power;
 			channels[j].channel = req->channels[i]->hw_value;
 
@@ -375,6 +372,7 @@ wl1271_scan_sched_scan_channels(struct wl1271 *wl,
 				struct wl1271_cmd_sched_scan_config *cfg)
 {
 	int idx = 0;
+	bool force_passive = !req->n_ssids;
 
 	cfg->passive[0] =
 		wl1271_scan_get_sched_scan_channels(wl, req, cfg->channels,
@@ -400,6 +398,14 @@ wl1271_scan_sched_scan_channels(struct wl1271 *wl,
 						    false, false, 14);
 	idx += cfg->active[1];
 
+	if (force_passive) {
+		/* move active channels to passive lists */
+		cfg->passive[0] += cfg->active[0] - 1;
+		cfg->active[0] = 1;
+		cfg->passive[1] += cfg->active[1];
+		cfg->active[1] = 0;
+	}
+
 	cfg->dfs =
 		wl1271_scan_get_sched_scan_channels(wl, req, cfg->channels,
 						    IEEE80211_BAND_5GHZ,
@@ -421,6 +427,7 @@ int wl1271_scan_sched_scan_config(struct wl1271 *wl,
 	struct wl1271_cmd_sched_scan_config *cfg = NULL;
 	struct conf_sched_scan_settings *c = &wl->conf.sched_scan;
 	int i, total_channels, ret;
+	bool force_passive = !req->n_ssids;
 
 	wl1271_debug(DEBUG_CMD, "cmd sched_scan scan config");
 
@@ -444,7 +451,7 @@ int wl1271_scan_sched_scan_config(struct wl1271 *wl,
 	for (i = 0; i < SCAN_MAX_CYCLE_INTERVALS; i++)
 		cfg->intervals[i] = cpu_to_le32(req->interval);
 
-	if (req->ssids[0].ssid_len && req->ssids[0].ssid) {
+	if (!force_passive && req->ssids[0].ssid_len && req->ssids[0].ssid) {
 		cfg->filter_type = SCAN_SSID_FILTER_SPECIFIC;
 		cfg->ssid_len = req->ssids[0].ssid_len;
 		memcpy(cfg->ssid, req->ssids[0].ssid,
@@ -461,7 +468,7 @@ int wl1271_scan_sched_scan_config(struct wl1271 *wl,
 		goto out;
 	}
 
-	if (cfg->active[0]) {
+	if (!force_passive && cfg->active[0]) {
 		ret = wl1271_cmd_build_probe_req(wl, req->ssids[0].ssid,
 						 req->ssids[0].ssid_len,
 						 ies->ie[IEEE80211_BAND_2GHZ],
@@ -473,7 +480,7 @@ int wl1271_scan_sched_scan_config(struct wl1271 *wl,
 		}
 	}
 
-	if (cfg->active[1]) {
+	if (!force_passive && cfg->active[1]) {
 		ret = wl1271_cmd_build_probe_req(wl,  req->ssids[0].ssid,
 						 req->ssids[0].ssid_len,
 						 ies->ie[IEEE80211_BAND_5GHZ],
-- 
1.7.1

Re: [PATCH 2.6.40] wl12xx: fix oops in sched_scan when forcing a passive scan

[Eliad Peller]

hi Luca,

On Wed, May 25, 2011 at 7:16 PM, Luciano Coelho <coelho@ti.com> wrote:
> Fix kernel oops when trying to use passive scheduled scans.  The
> reason was that in passive scans there are no SSIDs, so there was a
> NULL pointer dereference.
>
> To solve the problem, we now check the number of SSIDs provided in the
> sched_scan request and only access the list if there's one or more
> (ie. passive scan is not forced).  We also move the channels from
> active to passive if passive scanning is forced.  For this to work,
> it's necessary to set both active and passive dwell times for all
> channels.
>
> Signed-off-by: Luciano Coelho <coelho@ti.com>
> ---
[...]

why does sched scan without ssids means passive scan? can't we just do
active sched scan without ssids?

> +       if (force_passive) {
> +               /* move active channels to passive lists */
> +               cfg->passive[0] += cfg->active[0] - 1;
> +               cfg->active[0] = 1;
looks like a potential integer underflow.

if you're forcing a passive scan, why do you need to set an active channel?

anyway, this seems a bit wrong.
i don't think you can just do "arbitrary transfers" of the channel
counts, as their order seem to matter (i.e. the order of elements in
the channel array is passive[0],passive[1],..,active[0],active[1]...,
so you actually need to shift all the elements)

Eliad.

Re: [PATCH 2.6.40] wl12xx: fix oops in sched_scan when forcing a passive scan

[Luciano Coelho]

On Thu, 2011-05-26 at 02:08 +0300, Eliad Peller wrote:
> hi Luca,
> 
> On Wed, May 25, 2011 at 7:16 PM, Luciano Coelho <coelho@ti.com> wrote:
> > Fix kernel oops when trying to use passive scheduled scans.  The
> > reason was that in passive scans there are no SSIDs, so there was a
> > NULL pointer dereference.
> >
> > To solve the problem, we now check the number of SSIDs provided in the
> > sched_scan request and only access the list if there's one or more
> > (ie. passive scan is not forced).  We also move the channels from
> > active to passive if passive scanning is forced.  For this to work,
> > it's necessary to set both active and passive dwell times for all
> > channels.
> >
> > Signed-off-by: Luciano Coelho <coelho@ti.com>
> > ---
> [...]
> 
> why does sched scan without ssids means passive scan? can't we just do
> active sched scan without ssids?

This is how normal scans also work.  If you want a broadcast scan, you
pass a wildcard SSID (ie. one with zero length), if you want to force
the scan to be passive, you don't pass any SSIDs in the request.
Basically it means that if there is not SSID, we cannot send probe_req
(even broadcast probe_reqs need an SSID, an empty one).


> > +       if (force_passive) {
> > +               /* move active channels to passive lists */
> > +               cfg->passive[0] += cfg->active[0] - 1;
> > +               cfg->active[0] = 1;
> looks like a potential integer underflow.
> 
> if you're forcing a passive scan, why do you need to set an active channel?

Argh! This was from a test I was doing and is totally wrong.  The idea
is to transfer all the active channels to the passive list.  Thanks for
noticing this, I'll fix it.


> anyway, this seems a bit wrong.
>
> i don't think you can just do "arbitrary transfers" of the channel
> counts, as their order seem to matter (i.e. the order of elements in
> the channel array is passive[0],passive[1],..,active[0],active[1]...,
> so you actually need to shift all the elements)

Hmmm... Good point.  I don't know why the order would actually matter,
but in this case the way the channels are scanned will be hard to
predict.  We're doing something that is not specified or requested
anywhere.

This seemed to be the simplest way to do it, but indeed it's not very
good.  I'll have to do something so that it already sets all the
channels as passive when it generates the list, so no reordering or
hacking the channel numbers will be necessary.

Thanks for the review!

-- 
Cheers,
Luca.